[Nexthop] don't crash sw_agent in handlePendingUpdates when HwSwitch connections are lost

cmake/AgentTest.cmake

@@ -89,6 +89,23 @@ target_link_libraries(async_logger_test
 
 gtest_discover_tests(async_logger_test)
 
+add_executable(switch_handler_test
+  fboss/util/oss/TestMain.cpp
+  fboss/agent/test/SwitchHandlerTest.cpp
+)
+
+target_link_libraries(switch_handler_test
+  agent_test_utils
+  core
+  multi_switch_hw_switch_handler
+  multiswitch_service
+  common_utils
+  ${GTEST}
+  ${LIBGMOCK_LIBRARIES}
+)
+
+gtest_discover_tests(switch_handler_test)
+
 add_library(agent_test_lib
   fboss/agent/test/AgentTest.cpp
 )

fboss/agent/HwAgentMain.cpp

@@ -11,8 +11,10 @@
 #include <fb303/FollyLoggingHandler.h>
 #include <fb303/ServiceData.h>
 #include <folly/logging/Init.h>
+#include <folly/logging/LoggerDB.h>
 #include <folly/logging/xlog.h>
 #include "fboss/agent/AgentConfig.h"
+#include <gflags/gflags.h>
 #ifndef IS_OSS
 #include "common/fb303/cpp/DefaultControl.h"
 #include "common/fb303/cpp/DefaultMonitor.h"
@@ -79,6 +81,11 @@ void updateStats(
 namespace facebook::fboss {
 
 void SplitHwAgentSignalHandler::signalReceived(int /*signum*/) noexcept {
+  if (exitSignalReceived_.exchange(true)) {
+    XLOG(WARNING)
+        << "[Exit] Exit signal received while shutdown is already in progress, ignoring";
+    return;
+  }
   restart_time::mark(RestartEvent::SIGNAL_RECEIVED);
   XLOG(DBG2) << "[Exit] Signal received ";
   if (!hwAgent_->isInitialized()) {
@@ -155,8 +162,6 @@ void SplitHwAgentSignalHandler::signalReceived(int /*signum*/) noexcept {
     std::this_thread::sleep_for(std::chrono::seconds(FLAGS_agent_exit_delay_s));
     XLOG(INFO) << "[Exit] Delay complete, exiting now";
   }
-
-  exit(0);
 }
 
 int hwAgentMain(
@@ -313,7 +318,10 @@ int hwAgentMain(
   // @lint-ignore CLANGTIDY
   server->serve();
   server.reset();
-  return 0;
+  thriftSyncer.reset();
+  folly::LoggerDB::get().flushAllHandlers();
+  // NOLINTNEXTLINE(concurrency-mt-unsafe)
+  exit(signalHandler.exitSignalReceived() ? 0 : 1);
 }
 
 } // namespace facebook::fboss

fboss/agent/HwAgentMain.h

@@ -8,9 +8,8 @@
  *
  */
 #pragma once
+#include <atomic>
 #include <memory>
-
-#include <gflags/gflags.h>
 #include <string>
 
 #include "fboss/agent/CommonInit.h"
@@ -34,9 +33,14 @@ class SplitHwAgentSignalHandler : public SignalHandler {
 
   void signalReceived(int /*signum*/) noexcept override;
 
+  bool exitSignalReceived() const {
+    return exitSignalReceived_.load();
+  }
+
  private:
   std::unique_ptr<HwAgent> hwAgent_;
   SplitAgentThriftSyncer* syncer_;
+  std::atomic<bool> exitSignalReceived_{false};
 };
 
 void setSDKVersionInfo();

fboss/agent/HwSwitchConnectionStatusTable.cpp

@@ -70,7 +70,11 @@ bool HwSwitchConnectionStatusTable::disconnected(SwitchID switchId) {
                  << ": " << dirUtil->getHwColdBootOnceFile(switchIndex);
     }
 
-    std::exit(EXIT_SUCCESS);
+    // Schedule graceful teardown on the server's event base instead of exiting
+    // inline on this thrift stream cleanup thread.
+    lk.unlock();
+    sw_->requestGracefulShutdown();
+    return true;
   }
   if (FLAGS_exit_for_any_hw_disconnect) {
     XLOG(FATAL)
@@ -118,4 +122,9 @@ int HwSwitchConnectionStatusTable::getConnectionStatus(SwitchID switchId) {
   return connectedSwitches_.find(switchId) != connectedSwitches_.end() ? 1 : 0;
 }
 
+bool HwSwitchConnectionStatusTable::hasActiveConnections() {
+  std::lock_guard<std::mutex> lk(hwSwitchConnectedMutex_);
+  return !connectedSwitches_.empty();
+}
+
 } // namespace facebook::fboss

fboss/agent/HwSwitchConnectionStatusTable.h

@@ -25,6 +25,7 @@ class HwSwitchConnectionStatusTable {
   bool waitUntilHwSwitchConnected();
   void cancelWait();
   int getConnectionStatus(SwitchID switchId);
+  bool hasActiveConnections();
 
  private:
   std::set<SwitchID> connectedSwitches_;

fboss/agent/MultiHwSwitchHandler.cpp

@@ -273,6 +273,15 @@ bool MultiHwSwitchHandler::isHwSwitchConnected(const SwitchID& switchId) {
   return connectionStatusTable_.getConnectionStatus(switchId) == 1;
 }
 
+bool MultiHwSwitchHandler::hasActiveHwSwitchConnections() {
+  // for monolithic mode, we always return true, as we are not using
+  // connectionStatusTable_ in this case
+  if (sw_->isRunModeMonolithic()) {
+    return true;
+  }
+  return connectionStatusTable_.hasActiveConnections();
+}
+
 std::unique_ptr<TxPacket> MultiHwSwitchHandler::allocatePacket(uint32_t size) {
   // TODO - support with multiple switches
   CHECK_GE(hwSwitchSyncers_.size(), 1);

fboss/agent/MultiHwSwitchHandler.h

@@ -124,6 +124,7 @@ class MultiHwSwitchHandler {
   }
 
   bool isHwSwitchConnected(const SwitchID& switchId);
+  bool hasActiveHwSwitchConnections();
   void fillHwAgentConnectionStatus(AgentStats& agentStats);
 
   state::SwitchState reconstructSwitchState(SwitchID id);

fboss/agent/SwAgentInitializer.cpp

@@ -268,6 +268,13 @@ int SwAgentInitializer::initAgent(
 
   swHandler->setSSLPolicy(server_->getSSLPolicy());
 
+  // Register before start() so an all-HwSwitches-disconnected teardown can be
+  // scheduled on the event base. skipWarmBootStateSave=true: this is a cold
+  // shutdown whose cold-boot-once markers are already written.
+  sw_->registerGracefulShutdownHandler(eventBase_, [this]() {
+    handleExitSignal(true /* gracefulExit */, true /* skipWarmBootStateSave */);
+  });
+
   // At this point, we are guaranteed no other agent process will initialize
   // the ASIC because such a process would have crashed attempting to bind to
   // the Thrift port 5909
@@ -314,6 +321,6 @@ int SwAgentInitializer::initAgent(
     serverStarted_ = false;
   }
   serverStopCV_.notify_one();
-  return 0;
+  return exitStatus_.load();
 }
 } // namespace facebook::fboss

fboss/agent/SwAgentInitializer.h

@@ -10,6 +10,7 @@
 #include "fboss/agent/SwSwitch.h"
 
 #include <gflags/gflags.h>
+#include <atomic>
 #include <condition_variable>
 #include <mutex>
 
@@ -96,7 +97,9 @@ class SwAgentInitializer : public AgentInitializer {
   std::unique_ptr<SwSwitch> sw_;
   std::unique_ptr<SwSwitchInitializer> initializer_;
   std::shared_ptr<PacketStreamHandler> packetStreamHandler_;
-  virtual void handleExitSignal(bool gracefulExit) = 0;
+  virtual void handleExitSignal(
+      bool gracefulExit,
+      bool skipWarmBootStateSave = false) = 0;
 
   void stopServer();
   /*
@@ -109,6 +112,10 @@ class SwAgentInitializer : public AgentInitializer {
   virtual void stopServices();
   void waitForServerStopped();
 
+  // Exit status recorded by handleExitSignal() and returned by initAgent()
+  // once serve() unwinds.
+  std::atomic<int> exitStatus_{0};
+
  private:
   std::unique_ptr<apache::thrift::ThriftServer> server_;
   FbossEventBase* eventBase_;

fboss/agent/SwSwitch.cpp

@@ -227,6 +227,8 @@ facebook::fboss::PortStatus fillInPortStatus(
 }
 
 auto constexpr kHwUpdateFailures = "hw_update_failures";
+auto constexpr kHwUpdateDroppedNoConnection =
+    "hwswitch_disconnected_update_drop";
 
 std::string getDrainStateChangedStr(
     const std::shared_ptr<facebook::fboss::SwitchState>& oldState,
@@ -901,10 +903,12 @@ state::WarmbootState SwSwitch::gracefulExitState() const {
   return thriftSwitchState;
 }
 
-void SwSwitch::gracefulExit() {
+void SwSwitch::gracefulExit(bool skipWarmBootStateSave) {
   if (isFullyInitialized()) {
     steady_clock::time_point begin = steady_clock::now();
-    XLOG(DBG2) << "[Exit] Starting SwSwitch graceful exit";
+    XLOG(DBG2) << "[Exit] Starting SwSwitch graceful exit"
+               << (skipWarmBootStateSave ? " (skipping warm boot state save)"
+                                         : "");
     ipv6_->floodNeighborAdvertisements();
     arp_->floodGratuituousArp();
     steady_clock::time_point neighborFloodDone = steady_clock::now();
@@ -920,22 +924,27 @@ void SwSwitch::gracefulExit() {
                       stopThreadsAndHandlersDone - neighborFloodDone)
                       .count();
 
-    state::WarmbootState thriftSwitchState;
-    std::thread swWarmbootStateThread([this,
-                                       &thriftSwitchState,
-                                       stopThreadsAndHandlersDone]() {
-      thriftSwitchState = gracefulExitState();
-      steady_clock::time_point switchStateToThriftDone = steady_clock::now();
-      XLOG(DBG2) << "[Exit] Switch state to thrift "
-                 << duration_cast<duration<float>>(
-                        switchStateToThriftDone - stopThreadsAndHandlersDone)
-                        .count();
-    });
-    // Cleanup if we ever initialized
-    stopHwSwitchHandler();
+    if (!skipWarmBootStateSave) {
+      state::WarmbootState thriftSwitchState;
+      std::thread swWarmbootStateThread([this,
+                                         &thriftSwitchState,
+                                         stopThreadsAndHandlersDone]() {
+        thriftSwitchState = gracefulExitState();
+        steady_clock::time_point switchStateToThriftDone = steady_clock::now();
+        XLOG(DBG2) << "[Exit] Switch state to thrift "
+                   << duration_cast<duration<float>>(
+                          switchStateToThriftDone - stopThreadsAndHandlersDone)
+                          .count();
+      });
+      // Cleanup if we ever initialized
+      stopHwSwitchHandler();
 
-    swWarmbootStateThread.join();
-    storeWarmBootState(thriftSwitchState);
+      swWarmbootStateThread.join();
+      storeWarmBootState(thriftSwitchState);
+    } else {
+      // Cleanup if we ever initialized
+      stopHwSwitchHandler();
+    }
     XLOG(DBG2)
         << "[Exit] SwSwitch Graceful Exit time "
         << duration_cast<duration<float>>(steady_clock::now() - begin).count();
@@ -1402,6 +1411,27 @@ void SwSwitch::invokeNeighborListener(
   }
 }
 
+void SwSwitch::registerGracefulShutdownHandler(
+    FbossEventBase* evb,
+    std::function<void()> handler) {
+  gracefulShutdownEvb_ = evb;
+  gracefulShutdownHandler_ = std::move(handler);
+}
+
+void SwSwitch::requestGracefulShutdown() {
+  if (!gracefulShutdownHandler_ || !gracefulShutdownEvb_) {
+    XLOG(ERR)
+        << "requestGracefulShutdown called but no graceful shutdown handler registered";
+    return;
+  }
+  // Run teardown on the registered event base, not the caller's thread;
+  // once_flag collapses concurrent requests into a single shutdown.
+  std::call_once(gracefulShutdownOnceFlag_, [this]() {
+    gracefulShutdownEvb_->runInEventBaseThread(
+        [handler = gracefulShutdownHandler_]() { handler(); });
+  });
+}
+
 void SwSwitch::exitFatal() const noexcept {
   folly::dynamic switchState = folly::dynamic::object;
   // No hwswitch dump for multi swagent exit
@@ -2026,6 +2056,36 @@ void SwSwitch::handlePendingUpdates() {
           update->onError(ex);
         }
         return;
+      } else if (!multiHwSwitchHandler_->hasActiveHwSwitchConnections()) {
+        /*
+         * All HwSwitch connections are gone (e.g. hw_agent restarted right
+         * after sw_agent came up and the oper delta ack timed out or the
+         * stream disconnected). HwSwitchConnectionStatusTable::disconnected()
+         * has already created cold boot markers and scheduled a graceful
+         * shutdown, but the EXITING run state may not be set yet on this
+         * thread, so isExiting() can still be false here. Treat this like
+         * the exiting case instead of crashing: state will be resynced via
+         * the cold boot on restart.
+         *
+         * TODO: this connection-table check is a proxy for the
+         * HWSWITCH_STATE_UPDATE_CANCELLED status that
+         * MultiHwSwitchHandler::stateChanged computes per switch and then
+         * drops. It is correct only because both cancellation paths erase
+         * the connection-table entry before stateChanged returns to this
+         * thread. Propagating the aggregate update status out of
+         * MultiHwSwitchHandler::stateChanged would be exact, and would also
+         * let us handle partial cancellation (one of several HwSwitches
+         * cancelled) which today either still FATALs here or silently leaves
+         * the cancelled switch out of sync.
+         */
+        fb303::fbData->incrementCounter(kHwUpdateDroppedNoConnection);
+        XLOG(ERR) << "Failed to apply update to HW since all HwSwitch "
+                     "connections are lost; shutdown is in progress";
+        // Belt and braces: every path that empties the connection table
+        // already requests this (and call_once collapses the requests), but
+        // do not rely on that here - dropping updates without a pending
+        // teardown would leave a zombie agent acking updates forever.
+        requestGracefulShutdown();
       } else {
         XLOG(FATAL)
             << " Failed to apply update to HW and the update is not marked for "

fboss/agent/SwSwitch.h

@@ -848,8 +848,11 @@ class SwSwitch : public HwSwitchCallback {
   /*
    * Allow hardware to perform any cleanup needed to gracefully restart the
    * agent before we exit application.
+   *
+   * skipWarmBootStateSave: tear down without persisting warm-boot state or
+   * setting the can_warm_boot marker.
    */
-  void gracefulExit();
+  void gracefulExit(bool skipWarmBootStateSave = false);
 
   BootType getBootType() const {
     return bootType_;
@@ -879,6 +882,20 @@ class SwSwitch : public HwSwitchCallback {
       const std::vector<std::string>& added,
       const std::vector<std::string>& deleted);
 
+  /*
+   * Register a graceful shutdown handler, run on the given event base when
+   * requestGracefulShutdown() is called.
+   */
+  void registerGracefulShutdownHandler(
+      FbossEventBase* evb,
+      std::function<void()> handler);
+
+  /*
+   * Schedule the registered graceful shutdown handler. Safe to call from any
+   * thread; the handler runs at most once.
+   */
+  void requestGracefulShutdown();
+
   std::string getConfigStr() const;
   cfg::SwitchConfig getConfig() const;
   cfg::AgentConfig getAgentConfig() const;
@@ -1265,6 +1282,11 @@ class SwSwitch : public HwSwitchCallback {
   bool supportsAddRemovePort_;
   const std::unique_ptr<PlatformProductInfo> platformProductInfo_;
   std::atomic<SwitchRunState> runState_{SwitchRunState::UNINITIALIZED};
+
+  std::function<void()> gracefulShutdownHandler_{nullptr};
+  FbossEventBase* gracefulShutdownEvb_{nullptr};
+  std::once_flag gracefulShutdownOnceFlag_;
+
   folly::ThreadLocalPtr<SwitchStats, SwSwitch> stats_;
   /**
    * The object to sync the interfaces to the system. This pointer could