From 61922760b73ba527548449b00b9edc03cc7fb9df Mon Sep 17 00:00:00 2001 From: Sonic Build Admin Date: Fri, 24 Jul 2026 06:41:17 +0000 Subject: [PATCH] [orchagent]: _exit() on graceful shutdown to avoid destructor-chain memory corruption **What I did** Made orchagent exit through _exit(0) when a graceful shutdown is requested, instead of falling through to return 0 and running the full ~OrchDaemon destructor chain. Before exiting it drains the async swss recorder so any pending records are flushed first. **Why I did it** Fixes https://github.com/sonic-net/sonic-swss/issues/4600. PR #4400 added graceful shutdown handling. On SIGTERM or SIGINT the signal handler sets gOrchShutdownRequested, OrchDaemon::start() returns, and main() falls through to return 0. That runs the full ~OrchDaemon destructor chain, which is not safe to run on shutdown. During that teardown, FlexCounterManager destruction makes blocking SAI calls (stopFlexCounterPolling, which calls set_switch_attribute). Those calls go through sairedis's ZMQ channel to syncd and park the main thread in zmq_poll while several libzmq IO threads are still running. Orchs that were deleted earlier in the reverse order delete loop have freed buffers that those IO threads still reference, so the process corrupts its own heap. We saw orchagent crash in a libzmq IO thread when METADATA configuration was changed and the containers were restarted. **How I verified it** Configure the route state async publish and ZMQ options in METADATA, then restart the containers so orchagent receives SIGTERM. Before this change orchagent crashes fairly consistently during teardown. With this change orchagent shuts down cleanly and the crash no longer reproduces. **Details if related** Draining the async swss recorder through setAsync(false) joins the recorder worker so pending records are flushed, which was the reason PR #4400 wanted start() to return cleanly. Signed-off-by: Sonic Build Admin --- configure.ac | 2 +- orchagent/main.cpp | 16 +++++++++ orchagent/orchdaemon.cpp | 33 +++++++++++++++++++ orchagent/orchdaemon.h | 13 ++++++++ tests/mock_tests/orchdaemon_ut.cpp | 53 ++++++++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 63379cc0711..fe1e0a9e038 100644 --- a/configure.ac +++ b/configure.ac @@ -119,7 +119,7 @@ if test "x$enable_gcov" = "xyes"; then AC_MSG_ERROR(not compiling with gcc, which is required for gcov testing) fi - CFLAGS_COMMON+=" -fprofile-arcs -ftest-coverage" + CFLAGS_COMMON+=" -fprofile-arcs -ftest-coverage -DGCOV_ENABLED" AC_SUBST(CFLAGS_COMMON) LDFLAGS+=" -fprofile-arcs -lgcov" diff --git a/orchagent/main.cpp b/orchagent/main.cpp index 6e80c33449a..0f12694289c 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -1041,5 +1041,21 @@ int main(int argc, char **argv) orchDaemon->start(heartBeatInterval); + /* + * On SIGTERM/SIGINT the signal handler sets gOrchShutdownRequested and + * start() returns. Do not fall through to `return 0;`: running ~OrchDaemon + * and its member destructors is unsafe here. FlexCounterManager destruction + * issues SAI calls (stopFlexCounterPolling -> set_switch_attribute) that + * round-trip through sairedis's ZMQ channel and park the main thread in + * zmq_poll while libzmq I/O threads are still alive; orchs torn down earlier + * in the reverse-order loop have already freed buffers those threads still + * reference, corrupting the heap. + * + * Instead, drain the async swss recorder so pending records flush, then + * _exit() to let the kernel reclaim the rest of the process without the + * destructor chain. + */ + exit_if_graceful_shutdown_requested(); + return 0; } diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp index d9ab8f11870..f5af45aca86 100644 --- a/orchagent/orchdaemon.cpp +++ b/orchagent/orchdaemon.cpp @@ -1098,6 +1098,39 @@ void OrchDaemon::start(long heartBeatInterval) } } +#ifdef GCOV_ENABLED +extern "C" void __gcov_dump(void); +extern "C" void __gcov_reset(void); +#endif + +void exit_if_graceful_shutdown_requested(void (*exit_fn)(int)) +{ + if (gOrchShutdownRequested == 0) + { + return; + } + + SWSS_LOG_NOTICE("Exiting on graceful shutdown request (signal %d) without running destructors", gOrchShutdownRequested); + + /* + * Drain the async swss recorder before exiting: setAsync(false) stops the + * recorder worker only after any queued records have been written out. + */ + Recorder::Instance().swss.setAsync(false); + +#ifdef GCOV_ENABLED + /* + * _exit() skips libgcov's exit hook, so persist coverage data explicitly. + * Reset the counters afterwards so a unit-test caller passing a + * non-exiting exit_fn still dumps the remainder of its run at exit. + */ + __gcov_dump(); + __gcov_reset(); // LCOV_EXCL_LINE (resets its own arc counter, so it can never self-report) +#endif + + exit_fn(0); +} + /* * Try to perform orchagent state restore and dynamic states sync up if * warm start request is detected. diff --git a/orchagent/orchdaemon.h b/orchagent/orchdaemon.h index 6fbee416a0f..9d3028e621b 100644 --- a/orchagent/orchdaemon.h +++ b/orchagent/orchdaemon.h @@ -1,6 +1,8 @@ #ifndef SWSS_ORCHDAEMON_H #define SWSS_ORCHDAEMON_H +#include + #include "dbconnector.h" #include "producerstatetable.h" #include "consumertable.h" @@ -156,4 +158,15 @@ class DpuOrchDaemon : public OrchDaemon DBConnector *m_dpu_appDb; DBConnector *m_dpu_appstateDb; }; + +/* + * If a graceful shutdown was requested (SIGTERM/SIGINT set + * gOrchShutdownRequested and OrchDaemon::start() returned), drain the async + * swss recorder so pending records are flushed and terminate the process via + * exit_fn without running the OrchDaemon destructor chain. No-op when no + * shutdown was requested. exit_fn is injectable for unit tests; production + * callers use the default _exit. + */ +void exit_if_graceful_shutdown_requested(void (*exit_fn)(int) = _exit); + #endif /* SWSS_ORCHDAEMON_H */ diff --git a/tests/mock_tests/orchdaemon_ut.cpp b/tests/mock_tests/orchdaemon_ut.cpp index faa86ab2799..12cc1dc6516 100644 --- a/tests/mock_tests/orchdaemon_ut.cpp +++ b/tests/mock_tests/orchdaemon_ut.cpp @@ -8,9 +8,13 @@ #include "mock_sai_switch.h" #include "saihelper.h" +#include + extern sai_switch_api_t* sai_switch_api; sai_switch_api_t test_sai_switch; +extern volatile sig_atomic_t gOrchShutdownRequested; + namespace orchdaemon_test { @@ -248,4 +252,53 @@ namespace orchdaemon_test orchd->disableRingBuffer(); } + static int gMockExitCallCount; + static int gMockExitStatus; + + static void mock_exit_fn(int status) + { + gMockExitCallCount++; + gMockExitStatus = status; + } + + class GracefulShutdownExitTest : public ::testing::Test + { + protected: + void SetUp() override + { + gMockExitCallCount = 0; + gMockExitStatus = -1; + gOrchShutdownRequested = 0; + } + + void TearDown() override + { + gOrchShutdownRequested = 0; + Recorder::Instance().swss.setAsync(false); + } + }; + + TEST_F(GracefulShutdownExitTest, NoShutdownRequestedDoesNotExit) + { + Recorder::Instance().swss.setAsync(true); + + exit_if_graceful_shutdown_requested(mock_exit_fn); + + EXPECT_EQ(gMockExitCallCount, 0); + // The recorder must be left untouched when no shutdown was requested. + EXPECT_TRUE(Recorder::Instance().swss.isAsyncEnabled()); + } + + TEST_F(GracefulShutdownExitTest, ShutdownRequestDrainsRecorderAndExits) + { + Recorder::Instance().swss.setAsync(true); + gOrchShutdownRequested = SIGTERM; + + exit_if_graceful_shutdown_requested(mock_exit_fn); + + EXPECT_EQ(gMockExitCallCount, 1); + EXPECT_EQ(gMockExitStatus, 0); + EXPECT_FALSE(Recorder::Instance().swss.isAsyncEnabled()); + } + }