From c90280deb454090fe22cbe05dcfa213b06c9e3bd Mon Sep 17 00:00:00 2001 From: Robert M1 <50460704+githubrobbi@users.noreply.github.com> Date: Fri, 19 Jun 2026 07:31:50 -0700 Subject: [PATCH] fix(self-update): stop a non-elevated repair from fabricating a broker failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `uffs --update repair` (which runs NON-elevated by design — the whole point of the Access Broker) reported `[FAIL] Service restart failed: broker` while the very same report showed `[OK] Broker pipe serving`. The broker was healthy the entire time (confirmed: `uffs-broker --status` → running, serving). Root cause: the broker runs as a LocalSystem service. A non-elevated uffs-update cannot `OpenProcess` a SYSTEM-owned pid, so `proc::is_alive(pid)` false-negatives on it → the broker is flagged "down" → `restore` tries to `uffs_winsvc::start` it → that ALSO needs elevation → fails → a hard FAIL (exit 1) for a perfectly running broker. Two surgical fixes: - doctor `check_services`: determine the broker's liveness from its serving pipe (the authoritative signal, same probe `check_broker` uses), not from pid `OpenProcess` liveness. So a serving broker reads as "Service up", never enters the down/restart path. - restore `start_broker`: make it idempotent — if the broker is already serving, return success without an elevation-gated `uffs_winsvc::start`. This also covers the quiesce→restore path. Net: a non-elevated repair/restore no longer invents a broker failure when the broker is up; it only acts (and only warns, never hard-fails non-elevated) when the pipe is genuinely not serving. Verified: builds + 46 tests green, clippy clean, windows-msvc cross-compile OK. Co-Authored-By: Claude Opus 4.8 --- crates/uffs-update/src/doctor.rs | 13 ++++++++++++- crates/uffs-update/src/restore.rs | 8 ++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/uffs-update/src/doctor.rs b/crates/uffs-update/src/doctor.rs index 4b21bf2be..a6a0f7bca 100644 --- a/crates/uffs-update/src/doctor.rs +++ b/crates/uffs-update/src/doctor.rs @@ -382,7 +382,18 @@ fn count_stale_backups(dir: &Path) -> usize { fn check_services(snapshot: &plan::Snapshot, repair: bool, report: &mut Report) { let mut down = Vec::new(); for running in &snapshot.running { - if proc::is_alive(running.pid) { + // The broker runs as a LocalSystem service. A non-elevated uffs-update + // can't `OpenProcess` a SYSTEM-owned pid, so `proc::is_alive` false- + // negatives on it — which would flag a perfectly healthy broker as + // "down" and then trigger a doomed (also elevation-gated) restart. + // Its authoritative liveness is the serving pipe (what check_broker + // probes too), so use that for the broker instead of pid liveness. + let alive = if running.component == "broker" { + restore::broker_pipe_ready(DOCTOR_PIPE_PROBE_MS) + } else { + proc::is_alive(running.pid) + }; + if alive { report.add( Health::Ok, format!("Service up: {}", running.component), diff --git a/crates/uffs-update/src/restore.rs b/crates/uffs-update/src/restore.rs index 357045da9..d92162e8e 100644 --- a/crates/uffs-update/src/restore.rs +++ b/crates/uffs-update/src/restore.rs @@ -62,7 +62,15 @@ fn start_component(component: &str, running: &SnapRunning) -> bool { /// wait until the pipe is actually serving (R10, §19.13). Service-RUNNING /// is necessary but not sufficient: the daemon's warm-up hits /// `ERROR_PIPE_BUSY` if it connects before the broker's pipe is listening. +/// +/// Idempotent: if the broker is **already serving**, it's up — and a +/// non-elevated caller can neither need nor (via SCM) perform a start of a +/// `LocalSystem` service. Treat that as success so a redundant, elevation-gated +/// `uffs_winsvc::start` failure can't surface as a fault on a healthy broker. fn start_broker() -> bool { + if broker_pipe_ready(PIPE_READY_TIMEOUT_MS) { + return true; + } uffs_winsvc::start(SERVICE_NAME).is_ok() && broker_pipe_ready(PIPE_READY_TIMEOUT_MS) }