Preserve existing log transports when modifying log config (#1559)

Co-authored-by: AlCalzone <17641229+AlCalzone@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Dominic Griesel <dominic.griesel@nabucasa.com>

Author: Copilot

src/lib/driver/message_handler.ts

@@ -12,6 +12,7 @@ import { IncomingMessageDriver } from "./incoming_message.js";
 import { DriverResultTypes } from "./outgoing_message.js";
 import { dumpDriver, dumpLogConfig } from "../state.js";
 import { MessageHandler } from "../message_handler.js";
+import { preserveLogTransports } from "../logging.js";
 
 export class DriverMessageHandler implements MessageHandler {
   constructor(
@@ -47,7 +48,9 @@ export class DriverMessageHandler implements MessageHandler {
         return { config };
       }
       case DriverCommand.updateLogConfig: {
-        this.driver.updateLogConfig(message.config);
+        this.driver.updateLogConfig(
+          preserveLogTransports(this.driver, message.config),
+        );
         // If the logging event forwarder is enabled, we need to restart
         // it so that it picks up the new config.
         this.clientsController.restartLoggingEventForwarderIfNeeded();

src/lib/logging.ts

@@ -3,7 +3,7 @@ import { MESSAGE as messageSymbol } from "triple-beam";
 import { ConfigLogContext } from "@zwave-js/config";
 import { NodeLogContext } from "@zwave-js/core";
 import { createDefaultTransportFormat } from "@zwave-js/core/bindings/log/node";
-import type { ZWaveLogInfo } from "@zwave-js/core";
+import type { LogConfig, ZWaveLogInfo } from "@zwave-js/core";
 import { SerialLogContext } from "@zwave-js/serial";
 import { ClientsController, Logger } from "./server.js";
 import { ControllerLogContext, DriverLogContext, Driver } from "zwave-js";
@@ -35,23 +35,24 @@ export class LoggingEventForwarder {
   }
 
   start(filter?: Partial<LogContexts>) {
-    const { transports = [], level } = this.driver.getLogConfig();
+    const { level } = this.driver.getLogConfig();
     // Set the log level before attaching the transport
     this.logger.info("Starting logging event forwarder at " + level + " level");
     this.serverTransport = new WebSocketLogTransport(
       level as string,
       this.clients,
       filter,
     );
+    const transports = [...(this.driver.getLogConfig().transports ?? [])];
     transports.push(this.serverTransport);
     this.driver.updateLogConfig({ transports });
   }
 
   stop() {
     this.logger.info("Stopping logging event forwarder");
-    const transports = this.driver
-      .getLogConfig()
-      .transports.filter((transport) => transport !== this.serverTransport);
+    const transports = (this.driver.getLogConfig().transports ?? []).filter(
+      (transport) => transport !== this.serverTransport,
+    );
     this.driver.updateLogConfig({ transports });
     delete this.serverTransport;
   }
@@ -65,6 +66,16 @@ export class LoggingEventForwarder {
   }
 }
 
+export const preserveLogTransports = (
+  driver: Driver,
+  config: Partial<LogConfig>,
+): Partial<LogConfig> => {
+  if (!("transports" in config)) return config;
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  const { transports: _, ...rest } = config;
+  return { ...rest, transports: [...(driver.getLogConfig().transports ?? [])] };
+};
+
 class WebSocketLogTransport extends Transport {
   public constructor(
     level: string,

src/lib/server.ts

@@ -38,7 +38,11 @@ import {
 import { Instance } from "./instance.js";
 import { ServerCommand } from "./command.js";
 import { DriverMessageHandler } from "./driver/message_handler.js";
-import { LogContexts, LoggingEventForwarder } from "./logging.js";
+import {
+  LogContexts,
+  LoggingEventForwarder,
+  preserveLogTransports,
+} from "./logging.js";
 import { BroadcastNodeMessageHandler } from "./broadcast_node/message_handler.js";
 import { MulticastGroupMessageHandler } from "./multicast_group/message_handler.js";
 import { EndpointMessageHandler } from "./endpoint/message_handler.js";
@@ -165,7 +169,9 @@ export class Client {
       }
 
       if (msg.command === ServerCommand.updateLogConfig) {
-        this.driver.updateLogConfig(msg.config);
+        this.driver.updateLogConfig(
+          preserveLogTransports(this.driver, msg.config),
+        );
         this.sendResultSuccess(msg.messageId, {});
         return;
       }

src/mock/index.ts

@@ -18,19 +18,29 @@ class MockDriver extends EventEmitter {
 
   public statisticsEnabled = true;
 
+  private logConfig: Partial<LogConfig> = {
+    enabled: true,
+    level: "debug",
+    transports: [],
+  };
+
   async start() {
     this.emit("driver ready");
   }
 
   public getLogConfig(): Partial<LogConfig> {
     return {
-      enabled: true,
-      level: "debug",
-      transports: [],
+      ...this.logConfig,
+      transports: [...(this.logConfig.transports ?? [])],
     };
   }
 
-  public updateLogConfig(_config: Partial<LogConfig>) {}
+  public updateLogConfig(config: Partial<LogConfig>) {
+    this.logConfig = {
+      ...this.logConfig,
+      ...config,
+    };
+  }
 
   public updateUserAgent(
     _additionalUserAgentComponents?: Record<string, string> | null,

src/test/integration.ts

@@ -1,6 +1,8 @@
 import * as assert from "assert";
 import dns from "node:dns";
 import ws from "ws";
+import type { LogConfig } from "@zwave-js/core";
+import Transport from "winston-transport";
 import { libVersion } from "zwave-js";
 import { ZwavejsServer } from "../lib/server.js";
 import { createMockDriver } from "../mock/index.js";
@@ -11,6 +13,7 @@ const require = createRequire(import.meta.url);
 dns.setDefaultResultOrder("ipv4first");
 
 const PORT = 45001;
+type LogTransport = LogConfig["transports"][number];
 
 const createNextMessage = (socket: ws) => {
   let waitingListener: ((msg: unknown) => void) | undefined;
@@ -37,8 +40,50 @@ const createNextMessage = (socket: ws) => {
   };
 };
 
+const waitForResult = async (
+  nextMessage: ReturnType<typeof createNextMessage>,
+  messageId: string,
+) => {
+  for (;;) {
+    const message = await Promise.race([
+      nextMessage(),
+      new Promise((_, reject) =>
+        setTimeout(
+          () =>
+            reject(
+              new Error(
+                `Timed out while waiting for result message ${messageId}`,
+              ),
+            ),
+          5000,
+        ),
+      ),
+    ]);
+    if (
+      typeof message === "object" &&
+      message !== undefined &&
+      message !== null &&
+      "type" in message &&
+      message.type === "result" &&
+      "messageId" in message &&
+      message.messageId === messageId
+    ) {
+      return message;
+    }
+  }
+};
+
+class MockTransport extends Transport {
+  public log(_info: unknown, next: () => void): void {
+    next();
+  }
+}
+
 const runTest = async () => {
-  const server = new ZwavejsServer(createMockDriver(), { port: PORT });
+  const driver = createMockDriver();
+  const customTransport = new MockTransport() as LogTransport;
+  driver.updateLogConfig({ transports: [customTransport] });
+  const server = new ZwavejsServer(driver, { port: PORT });
   await server.start(true);
   let socket: ws | undefined = undefined;
 
@@ -97,6 +142,80 @@ const runTest = async () => {
       },
     });
 
+    socket.send(
+      JSON.stringify({
+        messageId: "start-listening-logs",
+        command: "driver.start_listening_logs",
+      }),
+    );
+
+    assert.deepEqual(await nextMessage(), {
+      type: "result",
+      success: true,
+      messageId: "start-listening-logs",
+      result: {},
+    });
+
+    assert.equal(
+      driver.getLogConfig().transports?.includes(customTransport),
+      true,
+    );
+    assert.equal(driver.getLogConfig().transports?.length, 2);
+
+    socket.send(
+      JSON.stringify({
+        messageId: "update-log-config",
+        command: "driver.update_log_config",
+        config: {
+          enabled: true,
+          level: "info",
+          transports: [],
+        },
+      }),
+    );
+
+    assert.deepEqual(await nextMessage(), {
+      type: "event",
+      event: {
+        source: "driver",
+        event: "log config updated",
+        config: {
+          enabled: true,
+          level: "info",
+        },
+      },
+    });
+
+    assert.deepEqual(await waitForResult(nextMessage, "update-log-config"), {
+      type: "result",
+      success: true,
+      messageId: "update-log-config",
+      result: {},
+    });
+
+    assert.equal(driver.getLogConfig().level, "info");
+    assert.equal(
+      driver.getLogConfig().transports?.includes(customTransport),
+      true,
+    );
+    assert.equal(driver.getLogConfig().transports?.length, 2);
+
+    socket.send(
+      JSON.stringify({
+        messageId: "stop-listening-logs",
+        command: "driver.stop_listening_logs",
+      }),
+    );
+
+    assert.deepEqual(await nextMessage(), {
+      type: "result",
+      success: true,
+      messageId: "stop-listening-logs",
+      result: {},
+    });
+
+    assert.deepEqual(driver.getLogConfig().transports, [customTransport]);
+
     console.log("Integration tests passed :)");
   } finally {
     if (socket) {