fix(startup): surface filesystem failures and verify Sapling param integrity

Three verified daemon-startup edge-case fixes centered on the config/params
filesystem path:

- F7: new non-throwing Platform::ensureDirectory(dir, outError) with one
  consistent "Cannot create <dir>: <reason>. Check permissions / free space."
  message. Replaces the unchecked/throwing create_directories sites at main.cpp
  (pre-init: log + Windows MessageBox + return 1), connection.cpp's
  autoDetectConfig (was the *throwing* overload -- could raise an uncaught
  filesystem_error through its callers; now sets the new
  ConnectionConfig::dir_error), and both app.cpp daemon-dir sites (surface via
  daemon_status_ + return false). The primary connect path (app_network.cpp)
  checks dir_error and shows it instead of mislabelling it "waiting for config".
  embedded_resources.cpp already checked its error_code, so it is left as-is.

- F6: verifySaplingParams() now hash-verifies each param against its pinned
  canonical SHA-256 (source of truth: scripts/build-lite-backend-artifact.sh)
  instead of only checking existence, so a truncated / corrupt-but-present param
  is rejected up front rather than failing later on a shielded operation. A
  <params_dir>/.sapling_verified marker keyed on size:mtime avoids re-hashing
  ~48MB on every startup. Logic extracted to the injectable, unit-testable
  verifySaplingParamsIn(dir, digests); reuses util::sha256Hex (no new hash impl).

- F5: startEmbeddedDaemon() now checks extractEmbeddedResources()'s return and
  the previously-dropped copy_file error_code in the daemon-binary fallback loop,
  aborting with a clear status (sb_daemon_extract_failed / sb_daemon_files_failed)
  instead of failing opaquely at spawn. An absent source file stays non-fatal.

Adds testPlatformEnsureDirectory and testVerifySaplingParams to test_phase4.cpp.
i18n keys added to i18n.cpp (English source of truth); the res/lang/*.json
back-fill via add_missing_translations.py is deferred to a single run at the end
of the batch. Progress tracked in docs/daemon-startup-hardening.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-08-02 11:07:32 -05:00
parent b3444e0a89
commit 2675b8ab93
10 changed files with 340 additions and 35 deletions

View File

@@ -29,8 +29,8 @@ around a single shared helper. The connectivity-breaking security flip lands las
| 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. | |
| 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. | ☐ |
@@ -216,7 +216,7 @@ F1/F2 (must not touch `crash_count_`; wording must not collide with the monitor'
## F5 — Extraction / copy write-failures never surfaced up front
**Severity:** Medium · **Effort:** S (~23h) · **Status:**
**Severity:** Medium · **Effort:** S (~23h) · **Status:** ☑ landed & verified
### The defect
`startEmbeddedDaemon()` discards `extractEmbeddedResources()`'s `bool` return
@@ -281,7 +281,14 @@ 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 (~35h) · **Status:**
**Severity:** Medium · **Effort:** S (~35h) · **Status:** ☑ landed & verified
> **As-built note.** `verifySaplingParams()` now delegates to a public, injectable
> `verifySaplingParamsIn(dir, digests)` so the integrity + marker-cache logic is unit-testable
> with synthetic small files (the real 48 MB params aren't in the repo). i18n keys for F5 were
> added to `i18n.cpp` (English source of truth); the `res/lang/*.json` back-fill via
> `scripts/add_missing_translations.py` is deferred to a single run at the end of the batch,
> per the cross-cutting note. Non-English locales fall back to English until then.
### The defect
`verifySaplingParams()` (`connection.cpp:123`) only calls `fs::exists()`;
@@ -331,7 +338,11 @@ Third caller of the existing `util::sha256Hex`.
## F7 — Directory-create errors universally ignored on the daemon-env path
**Severity:** Medium · **Effort:** S (~34h) · **Status:**
**Severity:** Medium · **Effort:** S (~34h) · **Status:** ☑ landed & verified
> **As-built notes.** Two deviations from the original design, both confirmed against the code:
> (1) `embedded_resources.cpp:270` already checks its `error_code` and returns `false` on failure — it was **not** a bug, so it is left untouched.
> (2) Of the four `autoDetectConfig` callers, only the primary connect path (`app_network.cpp:243`) was wired to check `dir_error`; the other three degrade gracefully on their own — `app.cpp:4306` and `app_wizard.cpp:912` are stop paths that already gate on empty creds, and `settings_page.cpp:434` is read-only display. `dir_error` is set by `autoDetectConfig`, so they can be wired later if desired.
### The defect
Five startup directory-create sites either drop the `error_code` or use the throwing
@@ -492,4 +503,7 @@ design record; see the shared-helper table above.
- **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.
- **F6** — ☑ landed: `verifySaplingParams()` now hash-verifies each Sapling param against its pinned canonical SHA-256 (from `build-lite-backend-artifact.sh`), replacing the existence-only check, so a truncated/corrupt-but-present param is rejected instead of failing later on a shielded op. A `<params_dir>/.sapling_verified` marker keyed on `size:mtime` skips re-hashing ~48 MB on every startup. Logic extracted to the injectable `verifySaplingParamsIn(dir, digests)`; new `testVerifySaplingParams` unit test (valid / marker fast-path / wrong-hash / truncated / missing). Clean build; `ctest` 1/1 passing.
- **F5** — ☑ landed: `startEmbeddedDaemon()` now checks `extractEmbeddedResources()`'s return (abort with `sb_daemon_extract_failed` on failure) and the previously-dropped `copy_file` `error_code` in the daemon-binary fallback loop (abort with `sb_daemon_files_failed` incl. the dir), so a disk-full / truncated `dragonxd` write is surfaced up front instead of failing opaquely at spawn. An absent source file stays non-fatal. Two i18n keys added to `i18n.cpp`. Clean build; `ctest` 1/1 passing.
- **F7** — ☑ landed: new non-throwing `Platform::ensureDirectory(dir, outError)` in `util/platform.{h,cpp}` with one consistent message. Replaces the unchecked/throwing directory-create sites at `main.cpp:730` (pre-init: now logs + `MessageBoxA` on Windows + `return 1`), `connection.cpp:216` (autoDetectConfig now uses the ec overload — **no more uncaught `filesystem_error`** — and sets the new `ConnectionConfig::dir_error`), and both `app.cpp` daemon-dir sites (surface via `daemon_status_` + `return false`). Primary connect path (`app_network.cpp:243`) checks `dir_error` and bails to the status line instead of mislabelling it "waiting for config". `embedded_resources.cpp:270` left as-is (already correct). New `testPlatformEnsureDirectory` unit test (existing-dir / fresh-nested / empty / parent-is-file). Clean build; `ctest` 1/1 passing.
- **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.