SIGHUP-based config reload for watch mode: address review feedback (#705)

* Initial plan

* Address review feedback: Kafka SSL context, SIGHUP handler safety, test formatting

Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com>
Agent-Logs-Url: https://github.com/domainaware/parsedmarc/sessions/8f2fd48f-32a4-4258-9a89-06f7c7ac29bf

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com>

parsedmarc/cli.py

@@ -961,7 +961,12 @@ def _init_output_clients(opts):
 
     if opts.kafka_hosts:
         ssl_context = None
-        if opts.kafka_ssl:
+        # SSL is used explicitly via kafka_ssl, or implicitly when credentials
+        # are provided (KafkaClient treats username/password as implying SSL).
+        kafka_uses_ssl = opts.kafka_ssl or bool(
+            opts.kafka_username or opts.kafka_password
+        )
+        if kafka_uses_ssl or opts.kafka_skip_certificate_verification:
             ssl_context = create_default_context()
             if opts.kafka_skip_certificate_verification:
                 logger.debug("Skipping Kafka certificate verification")
@@ -1988,8 +1993,9 @@ def _main():
 
     def _handle_sighup(signum, frame):
         nonlocal _reload_requested
+        # Logging is not async-signal-safe; only set the flag here.
+        # The log message is emitted from the main loop when the flag is read.
         _reload_requested = True
-        logger.info("SIGHUP received, config will reload after current batch")
 
     if hasattr(signal, "SIGHUP"):
         signal.signal(signal.SIGHUP, _handle_sighup)
@@ -2036,9 +2042,11 @@ def _main():
             if not _reload_requested:
                 break
 
-            # Reload configuration — clear the flag first so that any new
-            # SIGHUP arriving while we reload will be captured for the next
-            # iteration rather than being silently dropped.
+            # Reload configuration — emit the log message here (not in the
+            # signal handler, which is not async-signal-safe), then clear the
+            # flag so that any new SIGHUP arriving while we reload will be
+            # captured for the next iteration rather than being silently dropped.
+            logger.info("SIGHUP received, config will reload after current batch")
             _reload_requested = False
             logger.info("Reloading configuration...")
             try:

tests.py

@@ -1926,7 +1926,10 @@ password = pass
 watch = true
 """
 
-    @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform")
+    @unittest.skipUnless(
+        hasattr(signal, "SIGHUP"),
+        "SIGHUP not available on this platform",
+    )
     @patch("parsedmarc.cli._init_output_clients")
     @patch("parsedmarc.cli._parse_config_file")
     @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox")
@@ -1992,7 +1995,10 @@ watch = true
         # _parse_config_file called for initial load + reload
         self.assertGreaterEqual(mock_parse_config.call_count, 2)
 
-    @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform")
+    @unittest.skipUnless(
+        hasattr(signal, "SIGHUP"),
+        "SIGHUP not available on this platform",
+    )
     @patch("parsedmarc.cli._init_output_clients")
     @patch("parsedmarc.cli._parse_config_file")
     @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox")
@@ -2065,7 +2071,10 @@ watch = true
         # The failed reload must not have closed the original clients
         initial_clients["s3_client"].close.assert_not_called()
 
-    @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform")
+    @unittest.skipUnless(
+        hasattr(signal, "SIGHUP"),
+        "SIGHUP not available on this platform",
+    )
     @patch("parsedmarc.cli._init_output_clients")
     @patch("parsedmarc.cli._parse_config_file")
     @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox")
@@ -2137,7 +2146,10 @@ watch = true
         # Old client must have been closed when reload succeeded
         old_client.close.assert_called_once()
 
-    @unittest.skipUnless(hasattr(signal, "SIGHUP"), "SIGHUP not available on this platform")
+    @unittest.skipUnless(
+        hasattr(signal, "SIGHUP"),
+        "SIGHUP not available on this platform",
+    )
     @patch("parsedmarc.cli._init_output_clients")
     @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox")
     @patch("parsedmarc.cli.watch_inbox")