From 33fd08e2970d8fcb0c479b7a867d199a854a60cd Mon Sep 17 00:00:00 2001 From: CryptoKVC Date: Wed, 14 Oct 2026 14:59:29 +0200 Subject: [PATCH 1/2] Respect the log facade's level filters in LogFacadeWriter LogFacadeWriter passed records straight to `log::logger().log()`, so records above the facade's `max_level()` or `STATIC_MAX_LEVEL` still reached the application's logger. Apply the same checks as the `log` macros before building the record. The unit tests that use the global facade logger now share one logger and a lock, since the logger can only be set once per process. Written with the help of an AI assistant (Claude); reviewed and tested by me. --- src/logger.rs | 97 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 84 insertions(+), 13 deletions(-) diff --git a/src/logger.rs b/src/logger.rs index 857b33e3a9..cf455044b1 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -226,16 +226,23 @@ impl LogWriter for Writer { .expect("Failed to write to log file") }, Writer::LogFacadeWriter => { - let mut builder = LogFacadeRecord::builder(); - - match record.level { - LogLevel::Gossip | LogLevel::Trace => builder.level(LogFacadeLevel::Trace), - LogLevel::Debug => builder.level(LogFacadeLevel::Debug), - LogLevel::Info => builder.level(LogFacadeLevel::Info), - LogLevel::Warn => builder.level(LogFacadeLevel::Warn), - LogLevel::Error => builder.level(LogFacadeLevel::Error), + let level = match record.level { + LogLevel::Gossip | LogLevel::Trace => LogFacadeLevel::Trace, + LogLevel::Debug => LogFacadeLevel::Debug, + LogLevel::Info => LogFacadeLevel::Info, + LogLevel::Warn => LogFacadeLevel::Warn, + LogLevel::Error => LogFacadeLevel::Error, }; + // Apply the same gates as the `log` macros: the compile-time `STATIC_MAX_LEVEL` + // and the runtime maximum the application set via `log::set_max_level`. + if level > log::STATIC_MAX_LEVEL || level > log::max_level() { + return; + } + + let mut builder = LogFacadeRecord::builder(); + builder.level(level); + #[cfg(not(feature = "uniffi"))] log::logger().log( &builder @@ -311,7 +318,7 @@ impl LdkLogger for Logger { #[cfg(test)] mod tests { - use std::sync::Mutex; + use std::sync::{Mutex, OnceLock}; use super::*; #[cfg(unix)] @@ -349,6 +356,25 @@ mod tests { fn flush(&self) {} } + /// Serializes the tests that use the global `log` facade logger and maximum level. + static LOG_FACADE_LOCK: Mutex<()> = Mutex::new(()); + + /// Installs the global `TestLogger` once and returns the buffer it writes to. + /// + /// The `log` facade logger can only be set once per process, so every test that goes + /// through `Writer::LogFacadeWriter` shares this logger and holds `LOG_FACADE_LOCK`. + fn log_facade_test_output() -> Arc> { + static OUTPUT: OnceLock>> = OnceLock::new(); + OUTPUT + .get_or_init(|| { + let log = Arc::new(Mutex::new(String::new())); + log::set_boxed_logger(Box::new(TestLogger { log: log.clone() })) + .expect("no other log facade logger is set in unit tests"); + log + }) + .clone() + } + /// Tests that LogContext correctly formats all three structured fields /// (channel_id, peer_id, payment_hash) with space prefixes and 6-char truncation. #[test] @@ -403,10 +429,9 @@ mod tests { /// Tests that LogFacadeWriter appends structured context fields to the log message. #[test] fn test_log_facade_writer_includes_structured_context() { - let log = Arc::new(Mutex::new(String::new())); - let test_logger = TestLogger { log: log.clone() }; - - let _ = log::set_boxed_logger(Box::new(test_logger)); + let _guard = LOG_FACADE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let log = log_facade_test_output(); + let previous_max_level = log::max_level(); log::set_max_level(log::LevelFilter::Trace); let writer = Writer::LogFacadeWriter; @@ -448,5 +473,51 @@ mod tests { writer.log(record); assert_eq!(*log.lock().expect("lock"), "Test message (ch:abcdef p:02abcd)"); + log::set_max_level(previous_max_level); + } + + /// Tests that LogFacadeWriter drops records above the facade's maximum level, as the `log` + /// macros do, and still delivers records at or below it. + #[test] + fn test_log_facade_writer_respects_max_level() { + let _guard = LOG_FACADE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let log = log_facade_test_output(); + let previous_max_level = log::max_level(); + log::set_max_level(log::LevelFilter::Error); + + let writer = Writer::LogFacadeWriter; + let write = |level: LogLevel, message: &str| { + #[cfg(not(feature = "uniffi"))] + let record = LogRecord { + level, + args: format_args!("{}", message), + module_path: "test_module", + line: 42, + peer_id: None, + channel_id: None, + payment_hash: None, + }; + #[cfg(feature = "uniffi")] + let record = LogRecord { + level, + args: message.to_string(), + module_path: "test_module".to_string(), + line: 42, + peer_id: None, + channel_id: None, + payment_hash: None, + }; + writer.log(record); + }; + + log.lock().expect("lock").clear(); + write(LogLevel::Info, "info message"); + write(LogLevel::Gossip, "gossip message"); + assert_eq!(*log.lock().expect("lock"), ""); + + write(LogLevel::Error, "error message"); + assert_eq!(*log.lock().expect("lock"), "error message"); + + log::set_max_level(previous_max_level); } } From 0d6c9759c6f7d005088525aba84de25d227601bc Mon Sep 17 00:00:00 2001 From: CryptoKVC Date: Thu, 15 Oct 2026 16:11:25 +0200 Subject: [PATCH 2/2] Drop the logger test changes As requested in review, keep only the LogFacadeWriter change. --- src/logger.rs | 74 ++++----------------------------------------------- 1 file changed, 5 insertions(+), 69 deletions(-) diff --git a/src/logger.rs b/src/logger.rs index cf455044b1..c0b0082278 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -318,7 +318,7 @@ impl LdkLogger for Logger { #[cfg(test)] mod tests { - use std::sync::{Mutex, OnceLock}; + use std::sync::Mutex; use super::*; #[cfg(unix)] @@ -356,25 +356,6 @@ mod tests { fn flush(&self) {} } - /// Serializes the tests that use the global `log` facade logger and maximum level. - static LOG_FACADE_LOCK: Mutex<()> = Mutex::new(()); - - /// Installs the global `TestLogger` once and returns the buffer it writes to. - /// - /// The `log` facade logger can only be set once per process, so every test that goes - /// through `Writer::LogFacadeWriter` shares this logger and holds `LOG_FACADE_LOCK`. - fn log_facade_test_output() -> Arc> { - static OUTPUT: OnceLock>> = OnceLock::new(); - OUTPUT - .get_or_init(|| { - let log = Arc::new(Mutex::new(String::new())); - log::set_boxed_logger(Box::new(TestLogger { log: log.clone() })) - .expect("no other log facade logger is set in unit tests"); - log - }) - .clone() - } - /// Tests that LogContext correctly formats all three structured fields /// (channel_id, peer_id, payment_hash) with space prefixes and 6-char truncation. #[test] @@ -429,9 +410,10 @@ mod tests { /// Tests that LogFacadeWriter appends structured context fields to the log message. #[test] fn test_log_facade_writer_includes_structured_context() { - let _guard = LOG_FACADE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let log = log_facade_test_output(); - let previous_max_level = log::max_level(); + let log = Arc::new(Mutex::new(String::new())); + let test_logger = TestLogger { log: log.clone() }; + + let _ = log::set_boxed_logger(Box::new(test_logger)); log::set_max_level(log::LevelFilter::Trace); let writer = Writer::LogFacadeWriter; @@ -473,51 +455,5 @@ mod tests { writer.log(record); assert_eq!(*log.lock().expect("lock"), "Test message (ch:abcdef p:02abcd)"); - log::set_max_level(previous_max_level); - } - - /// Tests that LogFacadeWriter drops records above the facade's maximum level, as the `log` - /// macros do, and still delivers records at or below it. - #[test] - fn test_log_facade_writer_respects_max_level() { - let _guard = LOG_FACADE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let log = log_facade_test_output(); - let previous_max_level = log::max_level(); - log::set_max_level(log::LevelFilter::Error); - - let writer = Writer::LogFacadeWriter; - let write = |level: LogLevel, message: &str| { - #[cfg(not(feature = "uniffi"))] - let record = LogRecord { - level, - args: format_args!("{}", message), - module_path: "test_module", - line: 42, - peer_id: None, - channel_id: None, - payment_hash: None, - }; - #[cfg(feature = "uniffi")] - let record = LogRecord { - level, - args: message.to_string(), - module_path: "test_module".to_string(), - line: 42, - peer_id: None, - channel_id: None, - payment_hash: None, - }; - writer.log(record); - }; - - log.lock().expect("lock").clear(); - write(LogLevel::Info, "info message"); - write(LogLevel::Gossip, "gossip message"); - assert_eq!(*log.lock().expect("lock"), ""); - - write(LogLevel::Error, "error message"); - assert_eq!(*log.lock().expect("lock"), "error message"); - - log::set_max_level(previous_max_level); } }