From d188a08db7e2b8d9d64d1bbf3b6d2e50c5500a84 Mon Sep 17 00:00:00 2001 From: DanS Date: Sun, 2 Aug 2026 13:11:15 -0500 Subject: [PATCH] test(daemon): add F1/F2 process-lifecycle integration tests; fix clobbered start error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Links the real EmbeddedDaemon into the ObsidianDragonTests target (its deps were already present) and adds two POSIX integration tests that exercise the actual fork/exec/waitpid fixes headlessly: - testExecFailureReported (F2): start() against a non-executable file must fail with a precise "not executable or wrong architecture" reason. - testDaemonCrashDetected (F1): a short-lived child that exits abnormally is still detected (crash_count_ increments) while isRunning() is hammered from the test thread — a regression test for the reap race. Writing the F2 test surfaced a real bug: start()'s failure branch called setState(State::Error, "Failed to start dragonxd process"), and setState stores the Error message into last_error_ — clobbering the precise message startProcess() had just set, so getLastError()/the UI only ever saw the generic string. Fixed to pass the preserved detail to setState, so the precise reason survives and now also reaches the state callback (crash panel / status). ctest 1/1, green including the two new integration tests. Co-Authored-By: Claude Opus 4.8 --- CMakeLists.txt | 1 + docs/daemon-startup-hardening.md | 8 +++-- src/daemon/embedded_daemon.cpp | 10 ++++-- tests/test_phase4.cpp | 57 ++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5570548..d20bafb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1129,6 +1129,7 @@ if(BUILD_TESTING) src/data/address_book.cpp src/data/wallet_index.cpp src/daemon/lifecycle_adapters.cpp + src/daemon/embedded_daemon.cpp src/rpc/connection.cpp src/config/settings.cpp src/resources/embedded_resources.cpp diff --git a/docs/daemon-startup-hardening.md b/docs/daemon-startup-hardening.md index 22f1c15..d4a2818 100644 --- a/docs/daemon-startup-hardening.md +++ b/docs/daemon-startup-hardening.md @@ -24,9 +24,9 @@ back-fill applied additively to `res/lang/*.json` (42 keys — all 6 for es/de/f entries whose glyphs aren't in the current `NotoSansCJK-Subset.ttf` were left on English fallback rather than render as tofu). -**Still owed before release:** the F1/F2 manual repros (`kill -SEGV` / non-executable binary — not -unit-testable), and a **CJK subset-font rebuild** (`scripts/build_cjk_subset.py`, needs the Noto -CJK source font) to cover those 6 deferred zh/ja/ko strings. +**Still owed before release:** a **CJK subset-font rebuild** (`scripts/build_cjk_subset.py`, needs +the Noto CJK source font) to cover the 6 deferred zh/ja/ko strings. *(F1 and F2 now have headless +integration-test coverage — see the progress log — so their GUI repros are optional, not blocking.)* --- @@ -537,6 +537,8 @@ design record; see the shared-helper table above. ## Progress log +- **F1/F2 integration tests** — ☑ added `testExecFailureReported` (F2) and `testDaemonCrashDetected` (F1) to `test_phase4.cpp`, driving the **real** `EmbeddedDaemon` fork/exec/waitpid code headlessly (POSIX; required linking `embedded_daemon.cpp` into the test target — its deps were already there). The F1 test hammers `isRunning()` from the test thread while the child exits, so it's a genuine regression test for the reap race. **The F2 test caught a real bug:** `start()`'s failure branch overwrote `startProcess()`'s precise `last_error_` ("…not executable or wrong architecture") with a generic "Failed to start dragonxd process" (because `setState(Error, …)` stores its message into `last_error_`), so the precise reason never reached `getLastError()`/the UI — **fixed** to preserve the detail (now also surfaced via the state callback / crash panel). Build-clean; `ctest` 1/1. + - **F1** — ☑ landed: `isRunning()` (POSIX) now reads the atomic `state_` (predicate `Running || Stopping`) instead of calling `waitpid`, leaving `monitorProcess()` the sole reaper. Clean build (all targets link); `ctest` 1/1 passing. Not unit-testable — needs the manual `kill -SEGV` repro before release. - **F2** — ☑ landed: `startProcess()` (POSIX) now creates a `FD_CLOEXEC` self-pipe before `fork()`; the child writes `errno` to it on `execv` failure, the parent reads EOF-vs-errno and, on failure, reaps the zombie + sets a precise `last_error_` ("not executable or wrong architecture") + returns `false` (so `start()` no longer reports `Running` for a daemon that never started). Parent-side `setpgid` is now best-effort with a `DEBUG_LOGF` on failure. Clean build; `ctest` 1/1 passing. Not unit-testable — needs the manual non-executable / wrong-arch-binary repro before release. - **F8** — ☑ landed: `isLocalHost()` tightened to exact loopback via `isExactIPv4Loopback` (a `127.`-prefixed *hostname* like `127.evil.com` is no longer misclassified as local). `tryConnect()` now **refuses** a plaintext connection to a remote host instead of warn-and-proceeding — a local-network MITM can no longer capture `rpcuser:rpcpassword` — unless the user opts in with `rpcallowplaintext=1` in `DRAGONX.conf` (new `ConnectionConfig::allow_plaintext_remote` + `allowsPlaintextRemote()` policy). The refusal surfaces via status line + a one-time notification. New `testIsLocalHost` (12 assertions) + `testAllowsPlaintextRemote` (5). Clean build; `ctest` 1/1 passing. **Breaking — needs release notes; Settings-toggle UI deferred (see as-built note).** diff --git a/src/daemon/embedded_daemon.cpp b/src/daemon/embedded_daemon.cpp index 2add767..2444a79 100644 --- a/src/daemon/embedded_daemon.cpp +++ b/src/daemon/embedded_daemon.cpp @@ -585,8 +585,14 @@ bool EmbeddedDaemon::start(const std::string& binary_path) override_extra_args_.clear(); if (!startProcess(daemon_path, args)) { - DEBUG_LOGF("[ERROR] Failed to start dragonxd process: %s\\n", last_error_.c_str()); - setState(State::Error, "Failed to start dragonxd process"); + // startProcess() sets a precise last_error_ (e.g. "dragonxd could not be executed: + // ... not executable or wrong architecture"). Surface THAT via setState — which also + // stores the Error message into last_error_ — instead of clobbering it with a generic + // string that would then be all getLastError()/the UI ever sees. + std::string detail = last_error_.empty() ? std::string("Failed to start dragonxd process") + : last_error_; + DEBUG_LOGF("[ERROR] %s\n", detail.c_str()); + setState(State::Error, detail); return false; } diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index b5906ea..24cbab6 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -2634,6 +2634,59 @@ void testPlatformEnsureDirectory() } } +#ifndef _WIN32 +// Integration tests that drive the REAL EmbeddedDaemon fork/exec/waitpid paths (POSIX only). +void testExecFailureReported() +{ + using dragonx::daemon::EmbeddedDaemon; + namespace fsn = std::filesystem; + + // A present-but-non-executable file: execv() must fail, and the F2 self-pipe handshake + // must report it as a start FAILURE with a precise reason — not a transient "Running". + fsn::path bin = fsn::temp_directory_path() / "od_fake_daemon_bin"; + { std::ofstream(bin.string(), std::ios::binary) << "this is not an executable"; } + fsn::permissions(bin, fsn::perms::owner_read, fsn::perm_options::replace); // 0400, no +x + + EmbeddedDaemon d; + d.setSkipPortCheck(true); // bypass the port + datadir-lock gates so we reach startProcess() + EXPECT_TRUE(!d.start(bin.string())); + EXPECT_TRUE(d.getLastError().find("not executable or wrong architecture") != std::string::npos); + EXPECT_TRUE(!d.isRunning()); + + std::error_code ec; fsn::remove(bin, ec); +} + +void testDaemonCrashDetected() +{ + using dragonx::daemon::EmbeddedDaemon; + namespace fsn = std::filesystem; + + // A tiny script that ignores the injected daemon args, lives briefly, then exits abnormally + // — standing in for a daemon that crashes. is_script detection runs it via /bin/bash. + fsn::path script = fsn::temp_directory_path() / "od_fake_daemon.sh"; + { std::ofstream(script.string()) << "#!/bin/bash\nsleep 0.2\nexit 7\n"; } + fsn::permissions(script, fsn::perms::owner_all, fsn::perm_options::replace); // +x + + EmbeddedDaemon d; + d.setSkipPortCheck(true); + EXPECT_TRUE(d.start(script.string())); + EXPECT_TRUE(d.isRunning()); // reads the atomic state_, not a racy waitpid() + + // Hammer isRunning() the way the UI thread does while the child exits and monitorProcess() + // reaps it. Pre-fix (F1), isRunning()'s own waitpid() could steal the reap and hide the + // crash; with the fix the monitor is the sole reaper and always sees it. + for (int i = 0; i < 400 && d.getCrashCount() == 0; ++i) { + (void)d.isRunning(); + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + EXPECT_TRUE(d.getCrashCount() >= 1); // the unexpected exit was detected and counted + EXPECT_TRUE(!d.isRunning()); // state_ flipped to Error + + d.stop(); // join the monitor thread cleanly + std::error_code ec; fsn::remove(script, ec); +} +#endif // !_WIN32 + void testDatadirLockGate() { using dragonx::daemon::EmbeddedDaemon; @@ -6799,6 +6852,10 @@ int main() testWalletSecurityWorkflowExecutor(); testDaemonShutdownPolicy(); testDatadirLockGate(); +#ifndef _WIN32 + testExecFailureReported(); + testDaemonCrashDetected(); +#endif testPlatformEnsureDirectory(); testVerifySaplingParams(); testConnectHasStalled();