From 7ff356bddccdff922c217f64de8905a041076bf3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 08:38:09 -0400 Subject: [PATCH] fix(driver-cli-common): resolve Medium code-review finding (Driver.Cli.Common-003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConfigureLogging is now idempotent via a _loggingConfigured guard field so repeated calls from subclasses do not abandon and leak the previous logger. The previous Log.Logger is disposed before overwriting to release its console-sink resources cleanly. A new protected static FlushLogging() helper calls Log.CloseAndFlush() so commands can guarantee buffered output is flushed in their finally blocks before the process exits — important for the long-running subscribe verb. XML doc updated to reflect call-once semantics and document FlushLogging(). Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.Cli.Common/findings.md | 4 +-- .../DriverCommandBase.cs | 30 +++++++++++++++---- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/code-reviews/Driver.Cli.Common/findings.md b/code-reviews/Driver.Cli.Common/findings.md index cf991e2..356cc5f 100644 --- a/code-reviews/Driver.Cli.Common/findings.md +++ b/code-reviews/Driver.Cli.Common/findings.md @@ -105,7 +105,7 @@ on the masked code (`code & 0xFFFF0000`). | Severity | Medium | | Category | Concurrency & thread safety | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` | -| Status | Open | +| Status | Resolved | **Description:** `ConfigureLogging` assigns the process-global `Serilog.Log.Logger` without disposing the previously assigned logger and the library never calls @@ -121,7 +121,7 @@ command `ExecuteAsync`, or via a `protected` disposal helper on this base). Trea `ConfigureLogging` as call-once / idempotent and document that. At minimum capture and dispose the previous logger if reconfiguration is genuinely intended. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `ConfigureLogging` is now idempotent (guarded by `_loggingConfigured` field) and disposes the previous `Log.Logger` before overwriting; a new `protected static FlushLogging()` helper calls `Log.CloseAndFlush()` for commands to call in their `finally` blocks; XML doc updated accordingly. ### Driver.Cli.Common-004 diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs index a745322..68500f5 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs @@ -7,8 +7,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; /// /// Shared base for every driver test-client command (Modbus / AB CIP / AB Legacy / S7 / -/// TwinCAT). Carries the options that are meaningful regardless of protocol — verbose -/// logging + the standard timeout — plus helpers every command implementation wants: +/// TwinCAT / FOCAS). Carries the options that are meaningful regardless of protocol — +/// verbose logging + the standard timeout — plus helpers every command implementation wants: /// Serilog configuration + cancellation-token capture. /// /// @@ -44,17 +44,37 @@ public abstract class DriverCommandBase : ICommand public abstract ValueTask ExecuteAsync(IConsole console); /// - /// Configures the process-global Serilog logger. Commands call this at the top of - /// so driver-internal Log.Logger writes land on the - /// same sink as the CLI's operator-facing output. + /// Configures the process-global Serilog logger. Intended to be called exactly once, + /// at the top of , so driver-internal Log.Logger + /// writes land on the same sink as the CLI's operator-facing output. + /// If the logger has already been configured this call is a no-op (idempotent). + /// Call in a finally block to ensure buffered output + /// is flushed before the process exits. /// protected void ConfigureLogging() { + if (_loggingConfigured) return; + _loggingConfigured = true; + + // Dispose the previous global logger (e.g. Serilog's silent bootstrap logger) so + // its resources are released cleanly before we overwrite Log.Logger. + var previous = Log.Logger; var config = new LoggerConfiguration(); if (Verbose) config.MinimumLevel.Debug().WriteTo.Console(); else config.MinimumLevel.Warning().WriteTo.Console(); Log.Logger = config.CreateLogger(); + (previous as IDisposable)?.Dispose(); } + + /// + /// Flushes and closes the Serilog logger configured by . + /// Call this in a finally block inside to prevent + /// buffered log output from being lost on process exit, particularly for long-running + /// commands such as subscribe. + /// + protected static void FlushLogging() => Log.CloseAndFlush(); + + private bool _loggingConfigured; }