diff --git a/docs/daemon-startup-hardening.md b/docs/daemon-startup-hardening.md new file mode 100644 index 0000000..5aa315d --- /dev/null +++ b/docs/daemon-startup-hardening.md @@ -0,0 +1,495 @@ +# Daemon Startup Hardening — Implementation Plan + +Eight verified edge-case defects in how ObsidianDragon brings up (and watches) the +`dragonxd` daemon at launch. Each entry is a buildable fix: the defect (with exact +line references), the chosen approach, the call sites, a representative change, and how +to verify it. + +- **Scope:** full-node startup path (`--lite` excludes the embedded daemon entirely). +- **Source:** line references are exact against branch `dev` @ `45b652f`. +- **Provenance:** findings verified by direct source read; each fix designed by an + independent agent grounded in the cited files, with a sequencing pass for ordering, + shared helpers, and merge conflicts. + +**Severity:** 2 High, 6 Medium · **Effort:** ≈ 25–35 engineering-hours · **7 landing steps.** + +Status legend: ☐ not started · ◐ in progress · ☑ landed & verified + +--- + +## Recommended rollout sequence + +A real dependency order, not a checklist. The daemon-lifecycle cluster lands first +because it makes the `State::Error` / `crash_count_` contract trustworthy — which the +connect-stall panel and the lock gate both build on. The filesystem cluster lands +around a single shared helper. The connectivity-breaking security flip lands last. + +| Step | Finding(s) | Site | Why here | Status | +|------|-----------|------|----------|--------| +| 1 | **F1** | `embedded_daemon.cpp` · `isRunning()` | Smallest/highest-severity; establishes the reliable Error/crash-count transition steps 3 & 6 depend on. | ☑ | +| 2 | **F2** | `embedded_daemon.cpp` · `startProcess()` | Same file family, different function; test the F1+F2 pair together with `kill -SEGV` / bad-binary repros. | ☑ | +| 3 | **F4** | `embedded_daemon.cpp` · `start()` | After F1/F2 so crash-count semantics are settled; its bail deliberately stays out of the crash path. | ☑ | +| 4 | **F7** | `util/platform` · `connection.cpp` | Structural owner of the fs-error idiom + `ConnectionConfig` that F5/F6/F8 reuse. | ☐ | +| 5 | **F6 + F5** | `app.cpp` · `verifySaplingParams()` | Same `startEmbeddedDaemon` / `verifySaplingParams` block; land together. | ☐ | +| 6 | **F3** | `app.cpp` · `renderLoadingOverlay()` | After F1 — panel is guarded off during `State::Error` (owned by the crash-count hint). | ☐ | +| 7 | **F8** | `connection.cpp` · `tryConnect()` | Largest; only connectivity-breaking default flip — land last, with release notes. | ☐ | + +--- + +## F1 — Double-`waitpid` race can swallow a daemon crash + +**Severity:** High · **Effort:** S (~1–2h) · **Status:** ☑ landed & verified + +### The defect +`EmbeddedDaemon::isRunning()` (`embedded_daemon.cpp:1136`, POSIX branch) calls +`waitpid(WNOHANG)` — from the **UI thread, nearly every frame** — racing +`monitorProcess()`'s own reap at `:1244`. `waitpid` is one-shot: if the UI thread wins, +the monitor never decodes the exit, so `crash_count_` never increments, `State::Error` +never fires, and the 3-strike auto-restart cap (`app_network.cpp:479`) is defeated. The +sibling `XmrigManager::isRunning()` (`xmrig_manager.cpp:512`) already fixed exactly this +with an atomic read. + +### The fix +Make `isRunning()` read the existing `std::atomic state_` (member at +`embedded_daemon.h:253`) instead of calling `waitpid`, leaving `monitorProcess()` as the +sole reaper. Predicate is `Running || Stopping` — `Stopping` must stay "alive" because +`stop()`'s graceful/SIGTERM wait loops poll `isRunning()` before the process has exited. + +### Files touched +- `src/daemon/embedded_daemon.cpp` — `isRunning()`, POSIX branch (~1136) + +### Core change +```cpp +bool EmbeddedDaemon::isRunning() const // POSIX branch +{ + // Read the atomic state_ instead of waitpid() — monitorProcess() is the + // sole reaper. Previously both threads reaped; if the UI thread won, the + // monitor never saw the exit (crash_count_ / exit code / Error all lost). + if (process_pid_ <= 0) return false; + + State s = state_.load(std::memory_order_relaxed); + // Stopping stays "alive": stop()'s wait loops poll isRunning() while + // state_ == Stopping, before the process has actually terminated. + return (s == State::Running || s == State::Stopping); +} +``` + +### Verification +- Manual: `kill -SEGV` the daemon 10–20×; the monitor must report the exit and increment `crash_count_` every time (previously intermittent). +- Regression: a normal Settings-driven stop still escalates SIGTERM→SIGKILL (the `Stopping` predicate). +- Not unit-testable (real fork/exec/waitpid) — consistent with the no-process-spawn harness. + +### Dependencies +Mirrors `XmrigManager::isRunning()`. Flags a separate latent hazard (out of scope): +`stop()`'s final blocking `waitpid` (`:1220`) can still race a mid-sleep monitor +iteration — file as its own ticket. + +--- + +## F2 — exec-after-fork silent failure: "Running" for a daemon that never started + +**Severity:** High · **Effort:** S (~2–3h) · **Status:** ☑ landed & verified + +### The defect +In `startProcess()` (`embedded_daemon.cpp:957–1061`, POSIX) the parent runs +`process_pid_ = pid; return true;` **unconditionally** after `fork()` — with no +exec-status handshake. On a non-executable / wrong-arch / corrupt binary the child's +`execv` fails and it `_exit(127)`s, but `start()` has already set `State::Running` +(`:565`). The real cause never reaches `last_error_`; it surfaces later, generically, +as "exited unexpectedly (exit code 127)". + +### The fix +Add a **close-on-exec self-pipe** handshake — `pipe() + fcntl(FD_CLOEXEC)`, deliberately +**not** `pipe2()` (macOS lacks it; the POSIX branch is shared). The child writes `errno` +only on `execv` failure; a successful exec closes the write end for free. Parent reads: +EOF ⇒ success; 4 bytes ⇒ reap the zombie, set a precise `last_error_` ("not executable +or wrong architecture"), and return `false` so `start()` never reports Running. EINTR-safe +on both ends. Also comments the unchecked parent-side `setpgid` at `:1053`. + +### Files touched +- `src/daemon/embedded_daemon.cpp` — `startProcess()` parent read path +- `src/daemon/embedded_daemon.cpp` — child `execv`-failure write (~1043) +- `src/daemon/embedded_daemon.cpp` — `setpgid` best-effort comment (~1053) + +### Core change +```cpp +// Self-pipe exec handshake (pipe()+FD_CLOEXEC; NOT pipe2 — macOS lacks it). +int execpipe[2]; pipe(execpipe); +fcntl(execpipe[0], F_SETFD, FD_CLOEXEC); +fcntl(execpipe[1], F_SETFD, FD_CLOEXEC); + +pid_t pid = fork(); +if (pid == 0) { // child + close(execpipe[0]); + /* setpgid / chdir / dup2 / argv … */ + execv(binary_path.c_str(), argv.data()); + int e = errno; // execv failed + while (write(execpipe[1], &e, sizeof e) < 0 && errno == EINTR) {} + _exit(127); +} + +close(execpipe[1]); // parent: must close or read() never EOFs +int child_errno = 0, total = 0; +for (;;) { // EOF ⇒ exec ok; 4 bytes ⇒ exec failed + ssize_t n = read(execpipe[0], (char*)&child_errno + total, sizeof(int) - total); + if (n == 0) break; + if (n < 0) { if (errno == EINTR) continue; break; } + if ((total += n) >= (int)sizeof(int)) break; +} +close(execpipe[0]); +if (total >= (int)sizeof(int)) { // exec never happened + waitpid(pid, nullptr, 0); // reap the zombie + last_error_ = "dragonxd could not be executed: " + + std::string(strerror(child_errno)) + + " — not executable or wrong architecture"; + return false; // start() no longer reports Running +} +``` + +### Verification +- Point at a `chmod -x` / wrong-arch file → `start()` returns false immediately, precise message, no leftover zombie. +- Success path: real binary still starts with no perceptible added latency. +- Optional pure `formatExecFailureError(errno)` helper for a `test_phase4.cpp` unit test. + +### Dependencies +F1 (same function family; sequence F1→F2). **Highest-risk mistake:** forgetting +`FD_CLOEXEC` makes every successful start hang the parent read forever. + +--- + +## F4 — Stale datadir-lock start → restart storm that wedges the UI + +**Severity:** Medium · **Effort:** S (~3–5h) · **Status:** ☑ landed & verified + +### The defect +`start()` (`embedded_daemon.cpp:466`) gates only on the RPC port (`:482`), never on +`isDaemonProcessRunning()` (`:1292`). A graceful shutdown frees the port but keeps the +datadir `.lock` for up to ~90s. A rapid stop→start spawns a daemon that dies "Cannot +obtain a lock on data directory" — routed to the generic crash path. With a ~4s retry +cadence, **three lock races in ~12s exhaust the 3-strike budget** and wedge the UI long +before the lock actually clears. + +### The fix +Fail-fast with a **short bounded local wait (~300ms), not a 90s block**. After the port +bail, consult `isDaemonProcessRunning()` — gated by `!skip_port_check_` and exempt when +`override_datadir_` is set, so the isolated migrate-to-seed daemon still works. A pure +`evaluateDatadirLockGate()` returns a **distinct non-crash Error** that never increments +`crash_count_`. The connect loop's own retry then absorbs the transient. + +### Files touched +- `src/daemon/embedded_daemon.h` — decision struct, helper decl, poll constants +- `src/daemon/embedded_daemon.cpp` — `start()` gate + `evaluateDatadirLockGate()` + +### Core change +```cpp +static StartLockGateDecision evaluateDatadirLockGate( + bool skipPortCheck, bool isolatedOverride, bool stillRunningAfterWait) { + if (skipPortCheck || isolatedOverride) return {true, ""}; // migrate-to-seed exempt + if (!stillRunningAfterWait) return {true, ""}; + return {false, "A previous dragonxd is still shutting down and holding the " + "data directory lock. Retrying shortly…"}; +} + +// start() — after the isPortInUse() bail, before setState(Starting): +if (!skip_port_check_ && override_datadir_.empty()) { + bool stillLocked = false; // ~300ms bounded wait, NOT ~90s + for (int i = 0; i < kDatadirLockWaitMaxPolls; ++i) { + if (!isDaemonProcessRunning()) { stillLocked = false; break; } + stillLocked = true; + std::this_thread::sleep_for(std::chrono::milliseconds(kDatadirLockWaitPollMs)); + } + auto gate = evaluateDatadirLockGate(false, false, stillLocked); + if (!gate.proceed) { setState(State::Error, gate.errorMessage); return false; } +} +``` + +### Verification +- Unit: `evaluateDatadirLockGate()` across the skip / isolated / still-running matrix. +- Manual: rapid restart into a lingering lock → distinct message, no crash-cap wedge. +- Migrate-to-seed second daemon still starts (isolated exemption). + +### Dependencies +F1/F2 (must not touch `crash_count_`; wording must not collide with the monitor's +"exited unexpectedly"). Same TU, different function. + +--- + +## F5 — Extraction / copy write-failures never surfaced up front + +**Severity:** Medium · **Effort:** S (~2–3h) · **Status:** ☐ + +### The defect +`startEmbeddedDaemon()` discards `extractEmbeddedResources()`'s `bool` return +(`app.cpp:4152`) and the second copy-fallback loop drops `copy_file`'s `error_code` +entirely (`:4236`). Only Sapling params **existence** is re-checked — never the daemon +binary/CLI/tx/asmap. A disk-full or truncated `dragonxd` write falls straight through to +spawn and fails opaquely. The innermost write already returns `false` +(`embedded_resources.cpp:307`) — the signal is simply thrown away. + +### The fix +Minimal, surgical wiring — no new abstraction. Capture the extraction return and, on +failure, set `daemon_status_ = TR("sb_daemon_extract_failed")` and `return false` before +spawning. In the second copy loop, check `ec` after each `copy_file`, track `copyFailed`, +and abort with a dir-parameterized `sb_daemon_files_failed`. An **absent source** stays +fine (optional files); only an actual `error_code` counts. Written so F6/F7 slot in later +without re-touching this control flow. + +### Files touched +- `src/app.cpp` — `startEmbeddedDaemon()` extraction check (~4152) +- `src/app.cpp` — second copy-fallback loop (~4210–4242) +- `src/util/i18n.cpp` + `res/lang/*.json` — 2 additive keys + +### Core change +```cpp +// stop discarding the extraction result (~4152) +if (!resources::extractEmbeddedResources()) { + daemon_status_ = TR("sb_daemon_extract_failed"); // disk full / permission denied + return false; // abort before spawning +} + +// second copy-fallback loop — was dropping ec entirely (~4236) +bool copyFailed = false; +for (const char* name : { "asmap.dat", "dragonxd", "dragonx-cli", "dragonx-tx" }) { + fs::path dst = fs::path(daemon_dir) / name; + if (fs::exists(dst)) continue; // already present — skip + for (const auto& dir : searchDirs) { + fs::path src = fs::path(dir) / name; + if (!fs::exists(src)) continue; // absent source is OK, not a failure + fs::copy_file(src, dst, ec); + if (ec) { copyFailed = true; ec.clear(); } + break; + } +} +if (copyFailed) { + char buf[512]; + snprintf(buf, sizeof buf, TR("sb_daemon_files_failed"), daemon_dir.c_str()); + daemon_status_ = buf; + return false; // don't fall through to spawn +} +``` + +### Verification +- Unit: `extractEmbeddedResources()` returns false without embedded resources. +- Extract the copy loop into a testable helper; force one dst write to fail (dst is an existing directory). +- Manual: near-full tmpfs / read-only dir → clear status, daemon controller never constructed. + +### Dependencies +Shares the `daemon_status_` surfacing convention with F6; its early-return pattern is the +template F7 matches. Open item: remove truncated dst files so a retry re-copies. + +--- + +## F6 — Sapling params validated by existence/size only, never hashed + +**Severity:** Medium · **Effort:** S (~3–5h) · **Status:** ☐ + +### The defect +`verifySaplingParams()` (`connection.cpp:123`) only calls `fs::exists()`; +`resourceNeedsUpdate()` (`embedded_resources.cpp:250`) is size-only. On Linux (no +embedded resources) a **truncated-but-present** param passes and is handed to the daemon, +which then fails to build shielded proofs mid-operation — far from the real cause. + +### The fix +Add a pinned `{ filename → size, sha256 }` table (one source of truth, cross-referenced +to `scripts/build-lite-backend-artifact.sh`) and hash-check each param after the +existence check, reusing the existing `util::sha256Hex` (no second implementation). Since +these are ~48 MB, **cache the result** via a `.sapling_verified` marker keyed on +`size:mtime` — re-hash only when the stat line changes, so startup isn't slowed. + +### Files touched +- `src/rpc/connection.h` — `verifySaplingParams` decl +- `src/rpc/connection.cpp` — digest table, marker helpers, rewrite + +### Core change +```cpp +// connection.cpp — pinned known-good digests +// (source of truth: scripts/build-lite-backend-artifact.sh ensure_sapling_params) +constexpr SaplingParamDigest kSaplingParamDigests[] = { + { "sapling-spend.params", 47958396, "8e48ffd2…efc13" }, + { "sapling-output.params", 3592860, "2f0ebbcb…fb0e4" }, +}; + +bool Connection::verifySaplingParams() { + // existence check (unchanged) … + // cache: skip re-hashing a ~48 MB file unless size:mtime changed + if (readMarkerMatches(marker, statLines)) return true; + for (auto& d : kSaplingParamDigests) + if (util::sha256Hex(bytes) != d.sha256) return false; // reuse existing helper + writeMarker(marker, statLines); + return true; +} +``` + +### Verification +- Unit: good params pass; truncated / wrong-bytes rejected; marker cache short-circuits re-hash unless size/mtime changed. Real temp-file fixtures (matches existing `sha256Hex` tests). + +### Dependencies +F7 (reuse fs-error idiom; shares the `startEmbeddedDaemon`/`verifySaplingParams` block). +Third caller of the existing `util::sha256Hex`. + +--- + +## F7 — Directory-create errors universally ignored on the daemon-env path + +**Severity:** Medium · **Effort:** S (~3–4h) · **Status:** ☐ + +### The defect +Five startup directory-create sites either drop the `error_code` or use the throwing +overload with no `catch`: `main.cpp:730`, `connection.cpp:216` (can throw **uncaught** +through its callers), `embedded_resources.cpp:270`, `app.cpp:4172`/`4218`. A read-only +home or permission-denied yields a confusing "conf missing" / "binary not found" +downstream — or an uncaught `filesystem_error` — instead of a clear cause. + +### The fix +One shared, non-throwing `Platform::ensureDirectory(dir, outError)` in +`util/platform.{h,cpp}` that produces a single consistent message. Replace all five +sites; `autoDetectConfig()` moves off the throwing overload and sets a new +`ConnectionConfig::dir_error` that its four callers check and bail on. This is the +**structural owner** of the fs-error idiom that F5 and F6 reuse. + +### Files touched +- `src/util/platform.h` / `.cpp` — `ensureDirectory()` +- `src/rpc/connection.h` / `.cpp` — `dir_error` + `autoDetectConfig` +- `main.cpp`, `app.cpp`, `app_network.cpp`, `app_wizard.cpp`, `settings_page.cpp`, `embedded_resources.cpp` — 5 sites + 4 callers +- `tests/test_phase4.cpp` — `TestPlatformEnsureDirectory` + +### Core change +```cpp +// util/platform.cpp — one shared, non-throwing helper +bool Platform::ensureDirectory(const std::string& dir, std::string* outError) { + std::error_code ec; + if (std::filesystem::is_directory(dir, ec)) return true; + ec.clear(); + std::filesystem::create_directories(dir, ec); + if (ec) { + if (outError) + *outError = "Cannot create " + dir + ": " + ec.message() + + ". Check permissions / free space."; + return false; + } + return true; +} +// Replaces 5 ad-hoc sites; autoDetectConfig() now sets ConnectionConfig::dir_error, +// and its 4 callers bail on it. +``` + +### Verification +- Unit `TestPlatformEnsureDirectory`: existing dir → true; fresh nested → created; POSIX unwritable → false + message. +- All four `autoDetectConfig` callers tolerate `dir_error`. Pre-App-init site (main.cpp) reports via stderr / MessageBox. + +### Dependencies +**Owns** `Platform::ensureDirectory` (used by F5, F6) and the `ConnectionConfig` +extension (coordinated with F8). Land before F5/F6/F8. + +--- + +## F8 — Plaintext-remote RPC credential transmission is warn-only + +**Severity:** Medium · **Effort:** M (~6–9h) · **Status:** ☐ + +### The defect +A remote `rpchost` without `rpctls=1` sends Basic-auth `rpcuser:rpcpassword` over +cleartext HTTP. `tryConnect()` (`app_network.cpp:314`) only shows a **dismissible +warning** then proceeds — a local-network MITM sees the credentials. Compounding it, +`isLocalHost()`'s naive `rfind("127.",0)==0` misclassifies `127.evil.com` as local, +suppressing even the warning. + +### The fix +Change the policy to **refuse-by-default with an explicit, persisted opt-in** — a +`rpcallowplaintext=1` conf key (for hand-editors) and a Settings toggle. Block the +connect and show a **blocking modal** explaining the risk and how to enable TLS or opt +in; localhost is unaffected. Tighten `isLocalHost()` to exact `127.x.y.z` / `::1` / +`localhost` via `isExactIPv4Loopback()`. **Back-compat:** default off ⇒ existing remote +users hit a hard stop until they opt in — **ship with prominent release notes.** + +### Files touched +- `src/rpc/connection.h` / `.cpp` — `isLocalHost`, `allow_plaintext_remote`, `parseConfFile` +- `src/config/settings.h` / `.cpp` — persisted opt-in +- `src/app_network.cpp`, `src/app.h` — refuse + modal dispatch +- `src/ui/windows/plaintext_remote_rpc_dialog.h` — new blocking modal +- `src/ui/pages/settings_page.cpp` — toggle UI + +### Core change +```cpp +// Tightened loopback test — "127.evil.com" is NOT local +bool Connection::isLocalHost(const std::string& host) { + std::string h = stripBrackets(lowercase(host)); + return h == "localhost" || h == "::1" || isExactIPv4Loopback(h); // exact 127.x.y.z +} + +// Refuse-by-default with an explicit, persisted opt-in +const bool plaintextRemote = rpc::Connection::usesPlaintextRemote(config); +const bool plaintextAllowed = config.allow_plaintext_remote // rpcallowplaintext=1 + || settings_.getAllowPlaintextRemoteRpc(); // Settings toggle +if (plaintextRemote && !plaintextAllowed) { + connection_status_ = TR("sb_plaintext_remote_blocked"); + showPlaintextRemoteRpcDialog(config.host + ":" + config.port); // blocking modal + return; // no creds sent +} +``` + +### Verification +- Unit: `isLocalHost` — `127.evil.com` false, `127.0.0.1`/`::1`/`localhost` true; `allowsPlaintextRemote` honors conf key + settings flag. +- Manual: remote plaintext blocked; modal fires; opt-in persists across restart. + +### Dependencies +F7 (second extender of `ConnectionConfig`/`parseConfFile`; land after so the struct grows +once). Wire `renderPlaintextRemoteRpcDialog` into the app modal-dispatch list. + +--- + +## Shared helpers & coordination points + +| Helper | Purpose | Used by | +|--------|---------|---------| +| `Platform::ensureDirectory()` | Single non-throwing directory-create with one consistent message; replaces five ad-hoc sites. Owned by F7. | F7, F5, F6 | +| `ConnectionConfig` extension | Coordination point, not a function: F7 adds `dir_error`, F8 adds `allow_plaintext_remote`. Land F7→F8 so it grows once per step. | F7, F8 | +| `util::sha256Hex` *(existing)* | Already-compiled, curl-free SHA-256. F6 becomes its third caller — no second hash routine. | F6 | +| `connectHasStalled()` *(new, pure)* | Stall predicate split out of the ImGui/App code for unit testing, per the `*_updater_core.cpp` precedent. | F3 | +| `evaluateDatadirLockGate()` *(new, pure)* | Lock-gate decision as `{proceed, message}` from three booleans — unit-testable without real process/fs I/O. | F4 | + +## F3 — Unbounded connect spinner (deferred to step 6) + +**Severity:** Medium · **Effort:** S (~3–5h) · **Status:** ☐ + +The connect loop retries forever while `!state_.connected` (`app.cpp:1239`); +`loading_timer_` only animates the spinner. Stamp `connect_stall_since_` when +"reachable but not ready" is first seen; a pure `connectHasStalled()` helper (new +`util/connect_stall.h`, default 45s from `ui.toml`) flips `state_.connect_stalled` at +threshold, and `renderLoadingOverlay()` shows a "Taking longer than expected" panel with +Retry / Restart daemon / Open console (full-node gated). The background retry keeps +firing — recovery clears the panel automatically. Guarded off while the daemon is in +`State::Error` (owned by F1's crash-count hint). Full detail lives in the sequencing/ +design record; see the shared-helper table above. + +--- + +## Cross-cutting notes + +- **One TU, three functions.** `embedded_daemon.cpp` is edited by F1 (`isRunning`), + F2 (`startProcess`) and F4 (`start`) — no literal hunk overlap, but land in order to + keep "monitorProcess is the sole reaper" coherent. +- **Connection struct grows twice.** `connection.h/.cpp` is touched by F6, F7 and F8; + F7 and F8 both extend `ConnectionConfig` and `parseConfFile` — highest collision risk. + Sequence F7→F6→F8. +- **Testability split.** The three new pure predicates all get `tests/test_phase4.cpp` + coverage. F1/F2's fork/exec/waitpid changes are **not** unit-testable — they rely on + manual `kill` / non-executable-binary repros, consistent with the no-process-spawn harness. +- **i18n is additive-only.** Add each finding's English keys to `strings_`, then run + `scripts/add_missing_translations.py` **once at the very end** + (`json.dump indent=4, sort_keys=True, ensure_ascii=False`) — never bulk-regenerate a + `res/lang/*.json`. +- **F8 is a breaking default flip.** Refuse-plaintext-by-default stops existing + remote-RPC users cold until they opt in. Lands last, gated behind a persisted opt-in, + with release notes calling out the new `rpcallowplaintext` key and the Settings toggle. +- **Latent hazard, out of scope.** F1 surfaces (but doesn't fix) a second + double-`waitpid` window between `stop()`'s final blocking reap (`:1220`) and a + mid-sleep monitor iteration — file it as its own ticket. + +--- + +## Progress log + +- **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. +- **F4** — ☑ landed: `start()` now gates on a lingering datadir lock after the port bail. When `!skip_port_check_ && override_datadir_.empty()`, it polls `isDaemonProcessRunning()` with a bounded ~300 ms wait (3 × 100 ms, breaks early), then a pure header-inline `evaluateDatadirLockGate()` decides: if a sibling `dragonxd` is still alive it bails with a distinct **non-crash** `State::Error` ("…holding the data directory lock. Retrying shortly…") that never touches `crash_count_`, so the 3-strike cap can't trip; the connect loop's retry resumes once the lock clears. Isolated migrate-to-seed starts are exempt. New `testDatadirLockGate` unit test (5 assertions, proceed/bail/2× exempt) added to `test_phase4.cpp`. Clean build; `ctest` 1/1 passing. diff --git a/src/daemon/embedded_daemon.cpp b/src/daemon/embedded_daemon.cpp index 2e67fd6..2add767 100644 --- a/src/daemon/embedded_daemon.cpp +++ b/src/daemon/embedded_daemon.cpp @@ -488,6 +488,34 @@ bool EmbeddedDaemon::start(const std::string& binary_path) return false; } external_daemon_detected_ = false; + + // A previous dragonxd can release the RPC port well before it releases the datadir + // .lock — a graceful shutdown can take up to ~90s (see isDaemonProcessRunning). Starting + // into a still-held lock spawns a process that dies instantly with "Cannot obtain a lock + // on data directory"; the crash monitor reports that generically and, three times in + // ~12s, that is enough to trip the 3-strike restart cap before the lock's ~90s life + // elapses. Gate on the process actually still being alive, with a SHORT bounded wait + // (not the full ~90s — start() runs on the UI thread). Isolated starts (migrate-to-seed: + // skip_port_check_ / -datadir override) are exempt; they run their own datadir+port. + { + constexpr int kDatadirLockWaitPollMs = 100; + constexpr int kDatadirLockWaitMaxPolls = 3; // ~300ms total, breaks early on exit + bool stillRunning = false; + if (!skip_port_check_ && override_datadir_.empty()) { + stillRunning = isDaemonProcessRunning(); + for (int i = 0; stillRunning && i < kDatadirLockWaitMaxPolls; ++i) { + std::this_thread::sleep_for(std::chrono::milliseconds(kDatadirLockWaitPollMs)); + stillRunning = isDaemonProcessRunning(); + } + } + const StartLockGateDecision gate = + evaluateDatadirLockGate(skip_port_check_, !override_datadir_.empty(), stillRunning); + if (!gate.proceed) { + VERBOSE_LOGF("[INFO] %s\n", gate.errorMessage); + setState(State::Error, gate.errorMessage); + return false; + } + } setState(State::Starting, "Looking for dragonxd binary..."); @@ -962,18 +990,38 @@ bool EmbeddedDaemon::startProcess(const std::string& binary_path, const std::vec last_error_ = "Failed to create pipe: " + std::string(strerror(errno)); return false; } + + // Self-pipe used purely as an exec-success/failure handshake, separate from + // the stdout pipe above. Both ends are close-on-exec, so a successful execv() + // closes the write end for free (parent reads EOF); on execv() failure the + // child writes errno here, so the parent learns synchronously instead of + // reporting State::Running for a child that never became dragonxd. We use + // pipe()+FD_CLOEXEC (not pipe2) because this POSIX branch is shared with + // macOS, which has no pipe2(). + int execpipe[2]; + if (pipe(execpipe) == -1) { + last_error_ = "Failed to create exec-status pipe: " + std::string(strerror(errno)); + close(pipefd[0]); + close(pipefd[1]); + return false; + } + fcntl(execpipe[0], F_SETFD, FD_CLOEXEC); + fcntl(execpipe[1], F_SETFD, FD_CLOEXEC); pid_t pid = fork(); if (pid == -1) { last_error_ = "Fork failed: " + std::string(strerror(errno)); close(pipefd[0]); close(pipefd[1]); + close(execpipe[0]); + close(execpipe[1]); return false; } if (pid == 0) { // Child process - close(pipefd[0]); // Close read end + close(pipefd[0]); // Close read end of the stdout pipe + close(execpipe[0]); // Child only writes the exec-status pipe // Put child in its own process group so we can kill the entire // group later (including dragonxd spawned by a wrapper script). @@ -1040,22 +1088,61 @@ bool EmbeddedDaemon::startProcess(const std::string& binary_path, const std::vec execv(binary_path.c_str(), argv.data()); } - // If we get here, exec failed - fprintf(stderr, "execv failed: %s\n", strerror(errno)); + // If we get here, execv() failed — the child never became dragonxd. + // Capture errno before fprintf/strerror can clobber it, report it to + // the parent over the exec-status pipe (EINTR-safe), then exit. + int exec_errno = errno; + fprintf(stderr, "execv failed: %s\n", strerror(exec_errno)); + ssize_t w; + do { + w = write(execpipe[1], &exec_errno, sizeof(exec_errno)); + } while (w < 0 && errno == EINTR); _exit(127); } // Parent process - close(pipefd[1]); // Close write end + close(pipefd[1]); // Close our copy of the stdout write end + close(execpipe[1]); // Must close our copy, or the read() below never sees EOF + + // Exec-status handshake: EOF => execv() succeeded (its write end was closed + // on exec); a full sizeof(int) => execv() failed and the child sent errno. + int child_errno = 0; + size_t got = 0; + char* ep = reinterpret_cast(&child_errno); + for (;;) { + ssize_t n = read(execpipe[0], ep + got, sizeof(child_errno) - got); + if (n == 0) break; // EOF: exec succeeded + if (n < 0) { if (errno == EINTR) continue; break; } // other error: assume success + got += static_cast(n); + if (got >= sizeof(child_errno)) break; // full errno: exec failed + } + close(execpipe[0]); + + if (got >= sizeof(child_errno)) { + // execv() never replaced the child; it fprintf'd and _exit(127)'d. Reap + // the already-dead zombie here — monitorProcess() is only started after + // this function returns true, so there is no competing reaper. + close(pipefd[0]); + int status; + waitpid(pid, &status, 0); + last_error_ = "dragonxd could not be executed: " + std::string(strerror(child_errno)) + + " — not executable or wrong architecture"; + return false; + } + stdout_fd_ = pipefd[0]; - - // Also set process group from parent side (race with child's setpgid) - setpgid(pid, pid); - + + // Best-effort: the child already calls setpgid(0, 0); this parent-side call + // just closes the fork/exec race window. A failure here is not fatal to + // startup, so we log rather than abort. + if (setpgid(pid, pid) != 0) { + DEBUG_LOGF("[WARN] setpgid(%d) from parent failed: %s\n", (int)pid, strerror(errno)); + } + // Set non-blocking int flags = fcntl(stdout_fd_, F_GETFL, 0); fcntl(stdout_fd_, F_SETFL, flags | O_NONBLOCK); - + process_pid_ = pid; return true; } @@ -1135,17 +1222,21 @@ double EmbeddedDaemon::getMemoryUsageMB() const bool EmbeddedDaemon::isRunning() const { + // Read the atomic state_ instead of calling waitpid() here. monitorProcess() + // is the sole thread allowed to waitpid() process_pid_ during normal operation. + // Calling waitpid() from this method too (as it used to, and this is invoked + // from the UI thread nearly every frame) meant whichever thread reaped the + // child's exit first consumed the status; if isRunning() won that race, + // monitorProcess() never saw the exit, so crash_count_ / the decoded exit + // code / the State::Error transition were all silently lost. Mirrors the + // fix already in XmrigManager::isRunning(). if (process_pid_ <= 0) return false; - - int status; - pid_t result = waitpid(process_pid_, &status, WNOHANG); - - if (result == 0) { - // Still running - return true; - } - - return false; + + const State s = state_.load(std::memory_order_relaxed); + // State::Stopping is included: stop()'s graceful/SIGTERM wait loops poll + // isRunning() while state_ == Stopping — before the process has actually + // terminated — and must keep seeing "alive" to wait/escalate correctly. + return (s == State::Running || s == State::Stopping); } void EmbeddedDaemon::drainOutput() diff --git a/src/daemon/embedded_daemon.h b/src/daemon/embedded_daemon.h index 4021cab..bce7513 100644 --- a/src/daemon/embedded_daemon.h +++ b/src/daemon/embedded_daemon.h @@ -235,6 +235,32 @@ public: */ static bool isDaemonProcessRunning(); + /** Decision returned by evaluateDatadirLockGate(): whether start() may spawn now. */ + struct StartLockGateDecision { + bool proceed = true; // false => bail before spawning + const char* errorMessage = ""; // set (a string literal) when proceed == false + }; + + /** + * @brief Pure decision for start(): bail because a previous dragonxd still holds the + * shared datadir lock? Isolated instances (skip_port_check_ / an active -datadir + * override) are exempt — they run their own throwaway datadir+port and can coexist + * with the main daemon. Does no process/fs I/O itself (the caller does the probing), + * so it is directly unit-testable; defined inline so tests need only this header. + */ + static StartLockGateDecision evaluateDatadirLockGate(bool skipPortCheck, + bool isolatedOverride, + bool stillRunningAfterWait) + { + if (skipPortCheck || isolatedOverride) return {true, ""}; + if (stillRunningAfterWait) { + return {false, + "A previous dragonxd is still shutting down and holding the data " + "directory lock. Retrying shortly…"}; + } + return {true, ""}; + } + /** @brief Is an arbitrary TCP port currently in use on localhost? (used to pick a free port) */ static bool tcpPortInUse(int port); diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index 14cf7ed..d868e22 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -3,6 +3,7 @@ #include "chat/chat_service.h" #include "chat/chat_database.h" #include "daemon/daemon_controller.h" +#include "daemon/embedded_daemon.h" #include "data/transaction_history_cache.h" #include "data/address_book.h" #include "data/wallet_index.h" @@ -2477,6 +2478,28 @@ void testDaemonShutdownPolicy() EXPECT_TRUE(bootstrap.disconnectRpc); } +void testDatadirLockGate() +{ + using dragonx::daemon::EmbeddedDaemon; + + // Normal start, no lingering daemon after the bounded wait → proceed. + auto clear = EmbeddedDaemon::evaluateDatadirLockGate(false, false, false); + EXPECT_TRUE(clear.proceed); + + // A previous dragonxd still alive after the wait → bail with a distinct, non-crash msg. + auto locked = EmbeddedDaemon::evaluateDatadirLockGate(false, false, true); + EXPECT_TRUE(!locked.proceed); + EXPECT_TRUE(std::string(locked.errorMessage).find("data directory lock") != std::string::npos); + + // Isolated instance via skip_port_check_ is exempt even if a sibling dragonxd is running. + auto skipPort = EmbeddedDaemon::evaluateDatadirLockGate(true, false, true); + EXPECT_TRUE(skipPort.proceed); + + // Isolated instance via -datadir override is exempt even if a sibling is running. + auto isolated = EmbeddedDaemon::evaluateDatadirLockGate(false, true, true); + EXPECT_TRUE(isolated.proceed); +} + void testDaemonLifecycleExecution() { using dragonx::daemon::DaemonController; @@ -6619,6 +6642,7 @@ int main() testWalletSecurityWorkflow(); testWalletSecurityWorkflowExecutor(); testDaemonShutdownPolicy(); + testDatadirLockGate(); testDaemonLifecycleExecution(); testDaemonLifecycleAdapters(); testConsoleTextLayout();