From 2c04b88e19ba3d2b809bad6a376d5069179a5f4e Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 1 Oct 2026 09:02:45 -0500 Subject: [PATCH] fix(daemon): let POSIX stop() see the daemon exit instead of always SIGKILLing POSIX stop() set should_stop_, which made the monitor thread return without reaping the child, and then polled the state-based isRunning(), which stays true while the state is Stopping. So every Linux/macOS stop waited out its whole grace period, sent SIGTERM, waited 10 s more and SIGKILLed, even when the node had exited seconds earlier. Closing the GUI with an idle node took ~30 s; a node still flushing at 30 s was killed. - stop() joins the monitor first, then polls waitpid(pid, WNOHANG) on a cached pid, so it is the only reaper and sees the exit at once. - markStopRequested() (set by App::stopEmbeddedDaemon before the RPC stop, and by stop() itself; cleared by start()) makes an exit that lands before stop() runs a clean stop instead of a crash, on POSIX and in the Windows monitor. crash_count_ drives auto-restart, and a crash recorded before the request keeps its count. - The SIGTERM -> SIGKILL escalation stays for now, but only fires for a daemon that has really not exited. Replacing it with a wait policy and an explicit user choice is follow-up work. Verified against the fake daemon: an idle node that exits 3.4 s after stop used to be SIGTERMed at +20 s and SIGKILLed at +30 s (GUI closed in 30.6 s); it is now reaped as "exited cleanly" ~50 ms after it exits (GUI closes in 4.0 s). New POSIX tests cover the clean exit, the early requested exit and a graceful SIGTERM, each time-bounded so the old code fails fast. Co-Authored-By: Claude Opus 5.5 --- src/app.cpp | 4 + src/daemon/daemon_controller.cpp | 5 ++ src/daemon/daemon_controller.h | 1 + src/daemon/embedded_daemon.cpp | 123 +++++++++++++++++-------------- src/daemon/embedded_daemon.h | 18 ++++- tests/test_phase4.cpp | 95 ++++++++++++++++++++++++ 6 files changed, 187 insertions(+), 59 deletions(-) diff --git a/src/app.cpp b/src/app.cpp index b1ea337..ca4463c 100644 --- a/src/app.cpp +++ b/src/app.cpp @@ -5395,6 +5395,10 @@ void App::stopEmbeddedDaemon() // for it to disappear (handled after the stop() call). const bool owned = daemon_controller_->isRunning(); + // Any exit from here on is the stop we are asking for, not a crash — even one that lands before + // daemon_controller_->stop() below gets to reap it (crash_count_ drives auto-restart / give-up). + daemon_controller_->markStopRequested(); + // Send RPC "stop" command — this is the graceful path that lets the // daemon flush state, save block indexes, close sockets, etc. bool stop_sent = false; diff --git a/src/daemon/daemon_controller.cpp b/src/daemon/daemon_controller.cpp index 305656d..3391130 100644 --- a/src/daemon/daemon_controller.cpp +++ b/src/daemon/daemon_controller.cpp @@ -52,6 +52,11 @@ void DaemonController::stop(int waitMs) daemon_->stop(waitMs); } +void DaemonController::markStopRequested() +{ + daemon_->markStopRequested(); +} + bool DaemonController::isRunning() const { return daemon_->isRunning(); diff --git a/src/daemon/daemon_controller.h b/src/daemon/daemon_controller.h index d55ebfd..5a40b8f 100644 --- a/src/daemon/daemon_controller.h +++ b/src/daemon/daemon_controller.h @@ -90,6 +90,7 @@ public: bool start(const config::Settings* settings); void stop(int waitMs); + void markStopRequested(); bool isRunning() const; bool externalDaemonDetected() const; diff --git a/src/daemon/embedded_daemon.cpp b/src/daemon/embedded_daemon.cpp index c05c4f6..c90bb58 100644 --- a/src/daemon/embedded_daemon.cpp +++ b/src/daemon/embedded_daemon.cpp @@ -602,6 +602,7 @@ bool EmbeddedDaemon::start(const std::string& binary_path) override_datadir_.clear(); override_extra_args_.clear(); + stop_requested_ = false; // a new process: its exits are crashes until a stop is asked for if (!startProcess(daemon_path, args)) { // startProcess() sets a precise last_error_ (e.g. "dragonxd could not be executed: // ... not executable or wrong architecture"). Surface THAT via setState — which also @@ -971,6 +972,13 @@ void EmbeddedDaemon::monitorProcess() // Read any remaining output after process exits drainOutput(); + // An exit after markStopRequested() is the stop we asked for (it can land before stop() runs). + if (!should_stop_ && !isRunning() && stop_requested_) { + DEBUG_LOGF("dragonxd exited after a requested stop\n"); + setState(State::Stopped, "dragonxd stopped"); + return; + } + // Check if process exited unexpectedly if (!should_stop_ && !isRunning()) { // Get exit code for diagnostics @@ -1308,79 +1316,80 @@ void EmbeddedDaemon::drainOutput() } } +static std::string describeExitStatus(int status) +{ + if (WIFEXITED(status)) return "exit code: " + std::to_string(WEXITSTATUS(status)); + if (WIFSIGNALED(status)) return "killed by signal " + std::to_string(WTERMSIG(status)); + return "unknown exit status"; +} + +bool EmbeddedDaemon::reapWithin(pid_t pid, int timeout_ms, int& status) +{ + const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeout_ms); + for (;;) { + drainOutput(); + const pid_t r = waitpid(pid, &status, WNOHANG); + if (r == pid) return true; + if (r < 0 && errno != EINTR) return true; // ECHILD: nothing left to reap + if (std::chrono::steady_clock::now() >= deadline) return false; + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } +} + void EmbeddedDaemon::stop(int wait_ms) { + stop_requested_ = true; should_stop_ = true; - - if (process_pid_ > 0) { + + // Join the monitor FIRST (it sees should_stop_ within one 100ms tick) so that from here on this is + // the only reaper. isRunning() is state-based and stays true until we set Stopped below, so it can't + // tell us the child exited: poll waitpid() on a cached pid instead. If the child exited inside that + // tick, the monitor already reaped it and, seeing stop_requested_, recorded a clean stop (pid now 0). + if (monitor_thread_.joinable()) { + monitor_thread_.join(); + } + + const pid_t pid = process_pid_; + if (pid > 0) { setState(State::Stopping, "Stopping dragonxd..."); - + int status = 0; + // Phase 1: Wait for the daemon to exit naturally. // The caller (stopEmbeddedDaemon) already sent an RPC "stop" which // tells the daemon to flush LevelDB, close sockets, and exit cleanly. // On macOS/APFS the LevelDB flush can take several seconds — we must // NOT send SIGTERM until the daemon has had enough time to finish. DEBUG_LOGF("Waiting up to %d ms for daemon to exit after RPC stop...\n", wait_ms); - auto start = std::chrono::steady_clock::now(); - while (isRunning()) { - drainOutput(); - - auto elapsed = std::chrono::duration_cast( - std::chrono::steady_clock::now() - start).count(); - - if (elapsed >= wait_ms) { - break; - } - - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - } - - // Phase 2: If still running, send SIGTERM and wait a further 10s. - if (isRunning()) { - DEBUG_LOGF("Daemon did not exit gracefully — sending SIGTERM to process group -%d\n", process_pid_); - kill(-process_pid_, SIGTERM); - - auto sigterm_start = std::chrono::steady_clock::now(); - while (isRunning()) { - drainOutput(); - auto elapsed = std::chrono::duration_cast( - std::chrono::steady_clock::now() - sigterm_start).count(); - if (elapsed >= 10000) { - // Phase 3: Force kill - DEBUG_LOGF("Forcing dragonxd termination with SIGKILL (group -%d)...\n", process_pid_); - kill(-process_pid_, SIGKILL); - break; - } - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - } + if (reapWithin(pid, wait_ms, status)) { + DEBUG_LOGF("Daemon exited cleanly after RPC stop (%s)\n", describeExitStatus(status).c_str()); } else { - DEBUG_LOGF("Daemon exited cleanly after RPC stop\n"); + // Phase 2: still running — SIGTERM (dragonxd treats it like RPC stop) and a further 10s. + DEBUG_LOGF("Daemon did not exit gracefully — sending SIGTERM to process group -%d\n", pid); + kill(-pid, SIGTERM); + if (!reapWithin(pid, 10000, status)) { + // Phase 3: Force kill + DEBUG_LOGF("Forcing dragonxd termination with SIGKILL (group -%d)...\n", pid); + kill(-pid, SIGKILL); + while (waitpid(pid, &status, 0) < 0 && errno == EINTR) {} + } + DEBUG_LOGF("dragonxd exited (%s)\n", describeExitStatus(status).c_str()); } - drainOutput(); // read any remaining output - - // Reap the child process - int status; - waitpid(process_pid_, &status, 0); process_pid_ = 0; } - + if (stdout_fd_ >= 0) { close(stdout_fd_); stdout_fd_ = -1; } - - if (monitor_thread_.joinable()) { - monitor_thread_.join(); - } - + setState(State::Stopped, "dragonxd stopped"); } void EmbeddedDaemon::monitorProcess() { char buffer[4096]; - + while (!should_stop_) { // Check if process exited (non-blocking waitpid captures exit status) if (process_pid_ > 0) { @@ -1390,16 +1399,16 @@ void EmbeddedDaemon::monitorProcess() // Process exited — drain remaining output drainOutput(); - crash_count_++; - std::string exitInfo; - if (WIFEXITED(status)) { - exitInfo = "exit code: " + std::to_string(WEXITSTATUS(status)); - } else if (WIFSIGNALED(status)) { - exitInfo = "killed by signal " + std::to_string(WTERMSIG(status)); - } else { - exitInfo = "unknown exit status"; - } + const std::string exitInfo = describeExitStatus(status); process_pid_ = 0; + if (stop_requested_) { + // The stop we asked for (markStopRequested() / stop()) landed before stop() could + // reap it: a clean stop, not a crash. A crash recorded earlier keeps its count. + DEBUG_LOGF("dragonxd exited after a requested stop (%s)\n", exitInfo.c_str()); + setState(State::Stopped, "dragonxd stopped"); + return; + } + crash_count_++; setState(State::Error, "dragonxd exited unexpectedly (" + exitInfo + ")"); return; } diff --git a/src/daemon/embedded_daemon.h b/src/daemon/embedded_daemon.h index b2b8576..1ced1d7 100644 --- a/src/daemon/embedded_daemon.h +++ b/src/daemon/embedded_daemon.h @@ -59,7 +59,16 @@ public: * @param wait_ms Maximum milliseconds to wait for graceful shutdown */ void stop(int wait_ms = 5000); - + + /** + * @brief Record that we are about to ask the daemon to stop (e.g. over RPC) + * + * Call it before sending the stop. If the daemon then exits before stop() runs, the monitor + * records a clean stop instead of a crash (crash_count_ drives auto-restart). stop() sets it + * too; start() clears it. + */ + void markStopRequested() { stop_requested_ = true; } + /** * @brief Check if daemon process is running */ @@ -306,10 +315,15 @@ private: #else pid_t process_pid_ = 0; int stdout_fd_ = -1; + + // Waits up to timeout_ms for `pid` to exit, draining its output; true once reaped. Only valid + // after monitor_thread_ is joined, so that stop() is the sole reaper. + bool reapWithin(pid_t pid, int timeout_ms, int& status); #endif - + std::thread monitor_thread_; std::atomic should_stop_{false}; + std::atomic stop_requested_{false}; // a stop was asked for: the next exit is not a crash std::set debug_categories_; int max_connections_ = 0; // 0 = daemon default bool stratum_enabled_ = false; // -stratum: host a RandomX pool (v1.3.0+; older daemons ignore it) diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index be0df54..e8a8636 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -3181,9 +3181,101 @@ void testDaemonCrashDetected() EXPECT_TRUE(d.getCrashCount() >= 1); // the unexpected exit was detected and counted EXPECT_TRUE(!d.isRunning()); // state_ flipped to Error + const int crashes = d.getCrashCount(); d.stop(); // join the monitor thread cleanly + EXPECT_EQ(d.getCrashCount(), crashes); // a later stop never erases a real crash std::error_code ec; fsn::remove(script, ec); } + +// Writes an executable bash stand-in for dragonxd (EmbeddedDaemon runs it via /bin/bash); returns its path. +std::filesystem::path writeFakeDaemonScript(const std::string& name, const std::string& body) +{ + namespace fsn = std::filesystem; + fsn::path script = fsn::temp_directory_path() / name; + { std::ofstream(script.string()) << "#!/bin/bash\n" << body; } + fsn::permissions(script, fsn::perms::owner_all, fsn::perm_options::replace); // +x + return script; +} + +double secondsSince(std::chrono::steady_clock::time_point t0) +{ + return std::chrono::duration(std::chrono::steady_clock::now() - t0).count(); +} + +// S4: stop() must see a clean exit and return at once. Before the fix the monitor thread exited +// without reaping and the state-based isRunning() stayed true, so every POSIX stop waited out the +// whole grace period, then sent SIGTERM, then SIGKILL (here ~5 + 10 = 15s). +void testPosixStopReturnsAfterCleanExit() +{ + using dragonx::daemon::EmbeddedDaemon; + const auto script = writeFakeDaemonScript("od_fake_daemon_clean.sh", "sleep 1\nexit 0\n"); + + EmbeddedDaemon d; + d.setSkipPortCheck(true); + EXPECT_TRUE(d.start(script.string())); + const auto t0 = std::chrono::steady_clock::now(); + d.stop(5000); + EXPECT_TRUE(secondsSince(t0) < 2.5); + EXPECT_EQ(d.getCrashCount(), 0); + EXPECT_TRUE(d.getState() == EmbeddedDaemon::State::Stopped); + EXPECT_TRUE(!d.isRunning()); + std::error_code ec; std::filesystem::remove(script, ec); +} + +// S4: an exit after markStopRequested() is the stop we asked for, even when the monitor reaps it +// before stop() runs (the RPC stop landed first). It must not count as a crash or flip to Error. +void testPosixRequestedStopIsNotACrash() +{ + using dragonx::daemon::EmbeddedDaemon; + const auto script = writeFakeDaemonScript("od_fake_daemon_requested.sh", "sleep 0.3\nexit 0\n"); + + EmbeddedDaemon d; + d.setSkipPortCheck(true); + EXPECT_TRUE(d.start(script.string())); + d.markStopRequested(); + for (int i = 0; i < 300 && d.getState() == EmbeddedDaemon::State::Running; ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); // the monitor reaps it + EXPECT_TRUE(d.getState() == EmbeddedDaemon::State::Stopped); + EXPECT_EQ(d.getCrashCount(), 0); + + const auto t0 = std::chrono::steady_clock::now(); + d.stop(5000); + EXPECT_TRUE(secondsSince(t0) < 1.0); + EXPECT_EQ(d.getCrashCount(), 0); + std::error_code ec; std::filesystem::remove(script, ec); +} + +// S4: SIGTERM is graceful on POSIX (dragonxd treats it like RPC stop), and stop() reaps the clean +// exit it leads to at once instead of waiting out the 10s SIGKILL timer (~11s before the fix). +void testPosixStopSigtermIsGraceful() +{ + using dragonx::daemon::EmbeddedDaemon; + namespace fsn = std::filesystem; + const fsn::path events = fsn::temp_directory_path() / "od_fake_daemon_term.events"; + std::error_code ec; + fsn::remove(events, ec); + // Flushes for 1s on SIGTERM, then exits 0; otherwise never exits on its own. + const auto script = writeFakeDaemonScript("od_fake_daemon_term.sh", + "trap 'echo term >> \"$OD_TEST_EVENTS\"; sleep 1; echo clean >> \"$OD_TEST_EVENTS\"; exit 0' TERM\n" + "while :; do sleep 0.1; done\n"); + setenv("OD_TEST_EVENTS", events.string().c_str(), 1); + + EmbeddedDaemon d; + d.setSkipPortCheck(true); + EXPECT_TRUE(d.start(script.string())); + const auto t0 = std::chrono::steady_clock::now(); + d.stop(1000); // nothing asked it to stop, so SIGTERM goes out at +1s + EXPECT_TRUE(secondsSince(t0) < 4.0); + std::ifstream in(events); + const std::string seen((std::istreambuf_iterator(in)), std::istreambuf_iterator()); + EXPECT_EQ(seen, std::string("term\nclean\n")); // the trap ran to completion: no SIGKILL + EXPECT_EQ(d.getCrashCount(), 0); + EXPECT_TRUE(d.getState() == EmbeddedDaemon::State::Stopped); + + unsetenv("OD_TEST_EVENTS"); + fsn::remove(script, ec); + fsn::remove(events, ec); +} #endif // !_WIN32 void testDatadirLockGate() @@ -7753,6 +7845,9 @@ int main() #ifndef _WIN32 testExecFailureReported(); testDaemonCrashDetected(); + testPosixStopReturnsAfterCleanExit(); + testPosixRequestedStopIsNotACrash(); + testPosixStopSigtermIsGraceful(); #endif testPlatformEnsureDirectory(); testVerifySaplingParams();