From 051dd75b4025e0db439ea545a7b9e11bf7e0cfc8 Mon Sep 17 00:00:00 2001
From: Copilot <198982749+Copilot@users.noreply.github.com>
Date: Fri, 20 Mar 2026 19:31:16 -0400
Subject: [PATCH] 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 | 18 +++++++++++++-----
 tests.py          | 20 ++++++++++++++++----
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py
index 6c62ff1..eac2485 100644
--- a/parsedmarc/cli.py
+++ b/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:
diff --git a/tests.py b/tests.py
index 36389d2..faf18fa 100755
--- a/tests.py
+++ b/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")