Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Implement configurable native size/time log rotation & deletion - #189

Merged
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation
Jun 24, 2026
Merged

Implement configurable native size/time log rotation & deletion#189
benthecarman merged 1 commit into
lightningdevkit:mainfrom
Anyitechs:log-rotation

Conversation

@Anyitechs

@AnyitechsAnyitechs commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

This introduces a configurable native size/time log rotation & deletion functionality. It updates the ServerLogger to track file size and age internally, triggering rotation at a default size of 50MB or 24 hours, if the config values max_size_mb/rotation_interval_hours are unset. We keep only max_files (defaults to 5) rotated log files at a time, deleting older log files.

We also now allow users to turn off logging to file, but the default is logging to file and stdout/stderr. Internal rotation can be disabled for file logging by setting the max_size_mb and rotation_interval_hours params to 0

@ldk-reviews-bot

ldk-reviews-bot commented Apr 15, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@Anyitechs
Anyitechs marked this pull request as draft April 15, 2026 19:35
@benthecarman

Copy link
Copy Markdown
Collaborator

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

@AnyitechsAnyitechs changed the title refactor: encapsulate log rotation logicImplement native size/time log rotation with gzip compressionApr 17, 2026
@Anyitechs
Anyitechs marked this pull request as ready for review April 17, 2026 19:58
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

I would rather keep the old version. I like all the handling for the different signals in main.rs, makes things easier to reason about imo.

As we discussed elsewhere, this is now dropped entirely as we're moving to doing the rotation ourselves rather than relying on the user to configure logrotate.

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 1st Reminder

Hey @benthecarman! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment threadldk-server/src/util/logger.rs Outdated
Comment on lines +20 to +23
/// Maximum size of the log file before it gets rotated (50 MB)
const MAX_LOG_SIZE_BYTES: usize = 50 * 1024 * 1024;
/// Maximum age of the log file before it gets rotated (24 hours)
const ROTATION_INTERVAL_SECS: u64 = 24 * 60 * 60;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

these should be configurable

Comment threadldk-server/src/util/logger.rs Outdated
state.created_at = SystemTime::now();

// Spawn independent OS thread to compress the old file using native gzip
thread::spawn(move || match Command::new("gzip").arg("-f").arg(&rotated_path).status() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is assuming they have gzip installed and we have permissions to run it. I would rather just have the rust code do this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It doesn't really seem worth taking a gzip dependency just to do log rotation. We can just rotate and delete as-is and tell people to use the system log (which handles this properly) otherwise. Really we shouldn't be doing logging ourselves anyway - log should go to stdout/err and systemd or some other logic should figure out what to do with it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm happy to drop the compression so we do a rotate and delete. If that works for everyone, I'll update this to introduce a configurable max_rotated_files that will default to the last 5 uncompressed files. Does that work?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor of just doing deletion instead of compression but I would really rather keep the log file and not rely on just stdout/stderr. If we don't have an easy log file and rely on the user's systemd/docker/whatever to do and persist the logs, we will likely not have logs for lots of users and make it much harder to do support.

I'm also leaning towards just not logging to file. However I see your point that it might lead to nothing being logged at all. In that case we should however at the very least add the option to disable logging to file, and also update our docs and checked-in systemd ldk-server.service to use that mode, so that everybody who runs it that way will just use journald rather than our custom filesystem logger.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I like that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Just added an option to disable logging to file by default and a configurable native rotate and delete here b54e0a5

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
}

runtime.block_on(async {
// Register SIGHUP handler for log rotation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks like you actually removed the SIGHUP handling even though in the docs you say we have it

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, good catch. Thanks!

@AnyitechsAnyitechs changed the title Implement native size/time log rotation with gzip compressionImplement configurable native size/time log rotation & deletionMay 2, 2026
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed review comment here 42cd66a and rebased to fix conflict.

Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/operations.md Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threaddocs/configuration.md Outdated
}
}

fn flush(&self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex:

  • P1 File logs are now buffered with BufWriter (ldk-server/src/util/logger.rs:83), but normal writes do not flush (ldk-server/src/util/logger.rs:184) and shutdown does not call log::logger().flush() after the final log line (ldk-server/src/main.rs:527). Since the logger is installed
    globally, recent file logs from short runs or shutdown can remain in memory and never reach disk.

@benthecarmanbenthecarman mentioned this pull request May 7, 2026
25 tasks
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/main.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
log_max_size_bytes: usize,
log_rotation_interval_secs: u64,
log_max_files: usize,
internal_rotation_enabled: bool,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have internal_rotation_enabled but I don't see anyway to actually disable the rotation. Below its set to log_to_file so there is probably a bug here, either way its very confusing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Either way if we are going to keep the SIGHUP handling, we should have a way to disable the log rotation because otherwise the 2 systems would be fighting over each other. Made have it so if you set the param to 0 we don't rotate on that metric?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah true, thank you. I've adopted your suggestion and dropped the internal_rotation_enabled field entirely. We now allow disabling internal rotation by setting the log_max_size_bytes and log_rotation_interval_secs to 0 for users who want to keep logging to file but have something else do the rotation for them.

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and took the opportunity to squash fixup commits.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

few more small things and this should be good!

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/config.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
Comment threadldk-server/src/util/logger.rs Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Please excuse the delay here!

I've addressed all the latest comments here c8386a3. Let me know if/when I can squash.

Comment threadldk-server/src/util/logger.rs Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
Comment threaddocs/configuration.md Outdated
Comment threaddocs/operations.md Outdated
@Anyitechs

Copy link
Copy Markdown
ContributorAuthor

Addressed latest review comments and squashed fixups.

Ready for another look!

@benthecarmanbenthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

couple docs fixes then we're good

Comment threaddocs/operations.md Outdated
Comment threadcontrib/ldk-server-config.toml Outdated
This introduces a configurable native size/time log rotation & deletion
functionality. It updates the `ServerLogger` to track file size and age
internally, triggering rotation at a default size of 50MB or 24 hours,
if the config values `max_size_mb`/`rotation_interval_hours` are unset.
We keep only `max_files` (defaults to 5) rotated log files at a time,
deleting older log files.
We also now allow users to turn off logging to file, but the default is
logging to file and stdout/stderr. Internal rotation can be disabled for
file logging by setting the `max_size_mb` and `rotation_interval_hours`
params to `0`.
Co-Authored-By: Gemini 3.0 Pro
@benthecarman
benthecarman merged commit 628d95f into lightningdevkit:mainJun 24, 2026
8 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anyitechs@ldk-reviews-bot@benthecarman@tnull@TheBlueMatt