client: restore signal dispositions on teardown and escalate repeated signals

source/client/client.cc

@@ -87,19 +87,29 @@ bool Main::run() {
   OutputCollectorImpl output_collector(time_system, *options_);
   bool result;
   {
+    // The SignalHandler scope covers process->shutdown() as well: the shutdown/drain phase
+    // is the most likely to hang, and signals must keep being handled there. Requesting
+    // execution cancellation on an already-finished run is safe: worker cancellation and
+    // shutdown() serialize on the same lock inside ProcessImpl (encap subprocess
+    // termination is guarded by the runner's own mutex), and RemoteProcessImpl logs that
+    // cancellation is unsupported. The SignalHandler restores the previous (typically
+    // default) dispositions as soon as it consumes the first signal, so any further signal
+    // terminates the process (escalation) even if the cancellation callback blocks on a
+    // hung shutdown.
     auto signal_handler =
         std::make_unique<SignalHandler>([&process]() { process->requestExecutionCancellation(); });
     result = process->run(output_collector);
+    auto formatter = output_formatter_factory.create(options_->outputFormat());
+    absl::StatusOr<std::string> formatted_proto =
+        formatter->formatProto(output_collector.toProto());
+    if (!formatted_proto.ok()) {
+      ENVOY_LOG(error, "An error occurred while formatting proto");
+      result = false;
+    } else {
+      std::cout << *formatted_proto;
+    }
+    process->shutdown();
   }
-  auto formatter = output_formatter_factory.create(options_->outputFormat());
-  absl::StatusOr<std::string> formatted_proto = formatter->formatProto(output_collector.toProto());
-  if (!formatted_proto.ok()) {
-    ENVOY_LOG(error, "An error occurred while formatting proto");
-    result = false;
-  } else {
-    std::cout << *formatted_proto;
-  }
-  process->shutdown();
   if (!result) {
     ENVOY_LOG(error, "An error occurred.");
   } else {

source/common/signal_handler.cc

@@ -9,36 +9,77 @@ namespace Nighthawk {
 
 namespace {
 std::function<void(int)> signal_handler_delegate;
-void signal_handler(int signal) { signal_handler_delegate(signal); }
+// The delegate is cleared at the very end of ~SignalHandler(), after the dispositions have
+// been restored; the null check is defense-in-depth for the small window where a signal was
+// already dispatched into this handler while the destructor is swapping dispositions.
+void signal_handler(int signal) {
+  if (signal_handler_delegate) {
+    signal_handler_delegate(signal);
+  }
+}
 } // namespace
 
 SignalHandler::SignalHandler(const std::function<void()>& signal_callback) {
   pipe_fds_.resize(2);
   // The shutdown thread will be notified of by our signal handler and take it from there.
   RELEASE_ASSERT(pipe(pipe_fds_.data()) == 0, "pipe failed");
 
+  signal_handler_delegate = [this](int) { onSignal(); };
+  // Save the dispositions we replace so we can restore them after the first handled signal
+  // (second-signal escalation) and upon destruction. This happens before the shutdown
+  // thread is spawned so the thread is guaranteed to observe the saved values.
+  previous_sigterm_handler_ = signal(SIGTERM, signal_handler);
+  previous_sigint_handler_ = signal(SIGINT, signal_handler);
+  RELEASE_ASSERT(previous_sigterm_handler_ != SIG_ERR, "signal(SIGTERM) failed");
+  RELEASE_ASSERT(previous_sigint_handler_ != SIG_ERR, "signal(SIGINT) failed");
+
   shutdown_thread_ = std::thread([this, signal_callback]() {
     int tmp;
     RELEASE_ASSERT(read(pipe_fds_[0], &tmp, sizeof(int)) >= 0, "read failed");
     RELEASE_ASSERT(close(pipe_fds_[0]) == 0, "read side close failed");
     RELEASE_ASSERT(close(pipe_fds_[1]) == 0, "write side close failed");
     pipe_fds_.clear();
     if (!destructing_) {
+      // The first signal is about to be handled: fall back to the previous (typically
+      // default) dispositions BEFORE invoking the callback, so that any further signal
+      // gets the OS default action - giving the user a force-quit escalation path. This
+      // must happen before the callback rather than after it: the callback may itself
+      // block on the very shutdown sequence the user is trying to escape (e.g.
+      // requestExecutionCancellation() contends on the lock held across the worker drain
+      // loop in ProcessImpl::shutdown()), and a restore placed after it would never run
+      // in exactly that case. Calling signal() from this (non-main) thread is fine:
+      // POSIX specifies signal()/sigaction() as thread-safe (and async-signal-safe).
+      signal(SIGTERM, previous_sigterm_handler_);
+      signal(SIGINT, previous_sigint_handler_);
       signal_callback();
     }
   });
-
-  signal_handler_delegate = [this](int) { onSignal(); };
-  signal(SIGTERM, signal_handler);
-  signal(SIGINT, signal_handler);
 }
 
 SignalHandler::~SignalHandler() {
+  // Ordering rationale:
+  // 1) Set destructing_ first, so that if the shutdown thread gets woken up below (or by a
+  //    concurrently arriving signal) it will not invoke the callback mid-destruction.
+  // 2) Restore the previous signal dispositions. From this point on no NEW invocations of
+  //    the file-scope signal_handler() can start, so it becomes safe to tear down the
+  //    state it depends on. (If the shutdown thread already restored them after handling
+  //    a first signal, this re-installs the same values: harmless.)
+  // 3) Wake the shutdown thread if no signal ever fired (initiateShutdown() writes to the
+  //    pipe; it is a no-op when the thread already consumed a signal and closed the pipe)
+  //    and join it, draining the only thread that runs the callback or touches the pipe.
+  // 4) Only then clear signal_handler_delegate. Doing this any earlier could leave an
+  //    installed handler pointing at an empty std::function. Inherent (accepted) race: a
+  //    signal already dispatched into signal_handler() on another thread just before (2)
+  //    may still be executing; keeping the delegate alive until after restore+join makes
+  //    that window safe - the delegate only writes to the pipe, a no-op once closed.
   destructing_ = true;
+  signal(SIGTERM, previous_sigterm_handler_);
+  signal(SIGINT, previous_sigint_handler_);
   initiateShutdown();
   if (shutdown_thread_.joinable()) {
     shutdown_thread_.join();
   }
+  signal_handler_delegate = nullptr;
 }
 
 void SignalHandler::initiateShutdown() {

source/common/signal_handler.h

@@ -19,7 +19,10 @@ using SignalCallback = std::function<void()>;
  * Utility class for handling TERM and INT signals. Allows wiring up a callback that
  * should be invoked upon signal reception. This callback implementation does not have to be
  * signal safe, as a different thread is used to fire it.
- * NOTE: Only the first observed signal will result in the callback being invoked.
+ * NOTE: Only the first observed signal will result in the callback being invoked. After the
+ * first signal has been handled, the signal dispositions that were active before this
+ * instance was constructed are restored (typically SIG_DFL), so a second signal gets the
+ * OS default action (usually process termination) - an escalation path when shutdown hangs.
  * WARNING: only a single instance should be active at any given time in a process, and
  * the responsibility for not breaking this rule is not enforced at this time.
  *
@@ -70,6 +73,14 @@ class SignalHandler final : public Envoy::Logger::Loggable<Envoy::Logger::Id::ma
   // about signal-safety.
   std::vector<int> pipe_fds_;
   bool destructing_{false};
+
+  // The dispositions that were installed for SIGTERM/SIGINT before this instance replaced
+  // them (as returned by signal()). Restored after the first handled signal (so a second
+  // signal gets the OS default action) and again in the destructor. Relies on the existing
+  // single-active-instance assumption (see the class WARNING above); the file-scope
+  // delegate in signal_handler.cc already imposes it.
+  void (*previous_sigterm_handler_)(int) = nullptr;
+  void (*previous_sigint_handler_)(int) = nullptr;
 };
 
 using SignalHandlerPtr = std::unique_ptr<SignalHandler>;

test/common/signal_handler_test.cc

@@ -1,5 +1,9 @@
+#include <atomic>
+#include <chrono>
 #include <csignal>
 #include <future>
+#include <signal.h>
+#include <thread>
 
 #include "source/common/signal_handler.h"
 
@@ -31,5 +35,124 @@ TEST(SignalHandlerTest, DestructDoesNotFireHandler) {
   EXPECT_FALSE(signal_handled);
 }
 
+// Regression test: ~SignalHandler() used to leave the file-scope static `signal_handler`
+// installed for SIGTERM/SIGINT, whose delegate (the file-scope std::function
+// `signal_handler_delegate`) captured a `this` pointer to the DESTROYED SignalHandler
+// instance - a dangling delegate; any later signal was undefined behavior, best-case a
+// silent no-op. The destructor now restores the dispositions saved at construction time
+// (and clears the delegate), so after destruction SIGTERM/SIGINT are back to SIG_DFL.
+TEST(SignalHandlerTest, DestructorRestoresDefaultSignalDisposition) {
+  // Tests in this binary share a process, and earlier tests may already have left the
+  // stale nighthawk handler installed for these signals. To be independent of test
+  // ordering, force a known clean state (SIG_DFL) before constructing the handler under
+  // test. We intentionally do NOT reinstall the captured previous handlers afterwards:
+  // they may be the stale (dangling-delegate) handler, and SIG_DFL is the safe,
+  // non-polluting end state - every other test installs its own SignalHandler before
+  // raising signals.
+  const auto previous_term_disposition = std::signal(SIGTERM, SIG_DFL);
+  const auto previous_int_disposition = std::signal(SIGINT, SIG_DFL);
+  ASSERT_NE(previous_term_disposition, SIG_ERR);
+  ASSERT_NE(previous_int_disposition, SIG_ERR);
+
+  {
+    SignalHandler signal_handler([]() {});
+  }
+
+  // Query the current dispositions via a std::signal() round-trip: std::signal returns
+  // the handler that was installed at the time of the call.
+  const auto post_destruction_term = std::signal(SIGTERM, SIG_DFL);
+  const auto post_destruction_int = std::signal(SIGINT, SIG_DFL);
+  EXPECT_EQ(post_destruction_term, SIG_DFL)
+      << "~SignalHandler() left a stale SIGTERM handler installed";
+  EXPECT_EQ(post_destruction_int, SIG_DFL)
+      << "~SignalHandler() left a stale SIGINT handler installed";
+}
+
+// Second-signal escalation: after the FIRST signal is handled (the shutdown thread has
+// consumed it and run the callback), the SignalHandler restores the signal dispositions
+// that were active before it was constructed. A SECOND Ctrl-C therefore gets the OS
+// default action - process termination - instead of being silently swallowed, giving a
+// user stuck in a hanging shutdown/drain a force-quit escalation path.
+//
+// We deliberately do NOT raise a real second SIGTERM here: with the escalation in place
+// it would kill the test binary. Instead we prove the escalation by observing that the
+// installed disposition falls back to SIG_DFL. The restore runs on the shutdown thread
+// after the callback returns, so it is asynchronous with respect to the callback firing;
+// we poll for it with a bounded deadline. The poll uses sigaction() with a null
+// new-action, which is a pure READ of the current disposition - unlike a
+// std::signal(SIGTERM, <probe>) round-trip, it cannot race with (and clobber) the
+// shutdown thread's restore.
+TEST(SignalHandlerTest, SecondSignalFallsBackToDefaultDisposition) {
+  // Tests in this binary share a process; force a known clean state so the dispositions
+  // the SignalHandler under test saves (and later restores) are SIG_DFL, independent of
+  // test ordering.
+  ASSERT_NE(std::signal(SIGTERM, SIG_DFL), SIG_ERR);
+  ASSERT_NE(std::signal(SIGINT, SIG_DFL), SIG_ERR);
+
+  std::atomic<int> callback_count{0};
+  std::promise<void> first_callback_fired;
+
+  SignalHandler signal_handler([&callback_count, &first_callback_fired]() {
+    if (++callback_count == 1) {
+      first_callback_fired.set_value();
+    }
+  });
+
+  std::raise(SIGTERM);
+  first_callback_fired.get_future().wait();
+  EXPECT_EQ(callback_count, 1);
+
+  // Poll (read-only) until both dispositions are back to SIG_DFL, bounded at 2 seconds.
+  const auto dispositions_restored = []() {
+    struct sigaction term_action = {};
+    struct sigaction int_action = {};
+    if (sigaction(SIGTERM, nullptr, &term_action) != 0 ||
+        sigaction(SIGINT, nullptr, &int_action) != 0) {
+      return false;
+    }
+    return term_action.sa_handler == SIG_DFL && int_action.sa_handler == SIG_DFL;
+  };
+  // There is no injectable time source in SignalHandler; this bounded wall-clock poll is
+  // inherent to observing an asynchronous disposition change made by its shutdown thread.
+  const auto deadline =
+      std::chrono::steady_clock::now() + std::chrono::seconds(2); // NO_CHECK_FORMAT(real_time)
+  while (!dispositions_restored() &&
+         std::chrono::steady_clock::now() < deadline) {         // NO_CHECK_FORMAT(real_time)
+    std::this_thread::sleep_for(std::chrono::milliseconds(10)); // NO_CHECK_FORMAT(real_time)
+  }
+  EXPECT_TRUE(dispositions_restored())
+      << "second-signal escalation: SIGTERM/SIGINT dispositions were not restored to "
+         "SIG_DFL after the first signal was handled, so a second Ctrl-C would still be "
+         "silently swallowed";
+}
+
+// Regression death test: guards the dangling-delegate hazard from the process-lifetime
+// angle. ~SignalHandler() restores the saved dispositions (typically SIG_DFL), so raising
+// SIGTERM after destruction terminates the process. Before that fix, the stale static
+// handler swallowed the signal (invoking the dangling delegate - UB that in practice
+// wrote to a closed/cleared pipe and returned), the statement completed without dying,
+// and this test failed with "failed to die".
+//
+// The "threadsafe" death test style is required: it re-executes the test binary for
+// the child so the child starts single-threaded, which matters because SignalHandler
+// spawns a shutdown thread (the default "fast" style just fork()s a process that may
+// already have threads, which is unsafe/flaky here). This repo already uses death
+// tests elsewhere (e.g. test/rate_limiter_test.cc).
+TEST(SignalHandlerDeathTest, RaisingSignalAfterDestructionTerminatesProcess) {
+  const std::string previous_style = GTEST_FLAG_GET(death_test_style);
+  GTEST_FLAG_SET(death_test_style, "threadsafe");
+  EXPECT_DEATH(
+      {
+        {
+          SignalHandler signal_handler([]() {});
+        }
+        // With dispositions restored to SIG_DFL by the destructor above, this kills
+        // the child process (default action for SIGTERM).
+        std::raise(SIGTERM);
+      },
+      "");
+  GTEST_FLAG_SET(death_test_style, previous_style);
+}
+
 } // namespace
 } // namespace Nighthawk