From 7f1144542c5fa5f067862bf928beb5fbe6d4b969 Mon Sep 17 00:00:00 2001 From: Robert M1 <50460704+githubrobbi@users.noreply.github.com> Date: Wed, 8 Jul 2026 12:55:17 -0700 Subject: [PATCH 1/7] refactor(uninstall): adopt the shared elevation gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrate `uffs --uninstall` onto the shared `commands::elevation` gate that `uffs --update` already uses, so both flows share one implementation of "surface admin-only work up front, decide once". Behavior-preserving. - Extend the shared gate with `offer_inflow_elevation`: uninstall passes `true` (it has a one-shot elevated helper → Windows 3-way elevate/continue/ abort, `action = "removal"`); update passes `false` (no in-flow UAC → binary continue/abort). `GateWording` regains `action`; `ElevationChoice` regains `ElevateLater` (was uninstall's `ElevateAtRemoval`). - `RemovalPlan` now implements `ElevatablePlan` (its inherent `requires_elevation` / `drop_elevation_required` moved into the trait impl to satisfy `same_name_method`; `render_elevation_needed` delegates to `render::print_elevation_gate`). Call sites import the trait. - Delete uninstall's local `ElevationChoice` + `elevation_gate` + `platform_elevation_choice` (both cfg); the thin wrapper delegates to the shared gate. uninstall/mod.rs drops ~90 lines. 147 uffs-cli tests green (incl. the 12 uninstall plan tests on the now-trait methods); native + windows-msvc clippy + rustdoc clean. Co-Authored-By: Claude Opus 4.8 --- crates/uffs-cli/src/commands/elevation.rs | 100 ++++++++++++++--- crates/uffs-cli/src/commands/uninstall/mod.rs | 101 ++++-------------- .../uffs-cli/src/commands/uninstall/plan.rs | 38 ++++--- .../src/commands/uninstall/plan/tests.rs | 1 + .../uffs-cli/src/commands/uninstall/render.rs | 1 + crates/uffs-cli/src/commands/update/mod.rs | 6 +- 6 files changed, 137 insertions(+), 110 deletions(-) diff --git a/crates/uffs-cli/src/commands/elevation.rs b/crates/uffs-cli/src/commands/elevation.rs index 3604053c0..5fc93525f 100644 --- a/crates/uffs-cli/src/commands/elevation.rs +++ b/crates/uffs-cli/src/commands/elevation.rs @@ -1,20 +1,24 @@ // SPDX-License-Identifier: MPL-2.0 // Copyright (c) 2025-2026 SKY, LLC. -//! Shared elevation gate for the mutating flows. +//! Shared elevation gate for the mutating flows (`uffs --uninstall`, +//! `uffs --update`). //! //! A non-elevated run that has admin-only work is told **up front** what needs -//! Administrator and decides once — continue without the admin items (they are -//! dropped so the summary never lists work that will not happen), or abort and -//! re-run from an elevated terminal. Surfacing this before any mutation is what -//! keeps a flow from failing halfway through (the uninstall flow pioneered it; -//! `uffs --update` reuses this gate so both behave alike). +//! Administrator and decides once. Surfacing this before any mutation is what +//! keeps a flow from failing halfway through. Both flows drive this identically +//! so their elevation UX stays in lockstep. //! -//! Unlike the uninstall flow — which has a one-shot elevated helper it can -//! spawn via a single UAC prompt at removal time — the update flow has no -//! in-flow elevation (and `winget` itself *refuses* to upgrade a user package -//! from an elevated shell), so the elevated path here is simply "re-run -//! elevated". The choice is therefore binary on every platform. +//! The one difference is whether the flow can elevate **in place**: +//! +//! - `uffs --uninstall` has a one-shot elevated helper it spawns via a single +//! UAC prompt at removal time, so on Windows it offers a 3-way choice — +//! elevate now / continue without / abort (`offer_inflow_elevation = true`). +//! - `uffs --update` has no in-flow elevation (and `winget` itself *refuses* to +//! upgrade a user package elevated), so its elevated path is simply "re-run +//! elevated" and the choice stays binary (`offer_inflow_elevation = false`). +//! +//! Non-Windows has no UAC to request, so the choice is always binary there. use anyhow::{Context as _, Result, bail}; @@ -34,8 +38,18 @@ pub(crate) trait ElevatablePlan { /// Flow-specific wording woven into the gate prompt. pub(crate) struct GateWording { - /// The command to re-run elevated — `"uffs --update"` / `"uffs - /// --uninstall"`. + /// The mutating action — `"removal"` / `"update"` — used in + /// "elevate at <action> time" (only on the in-flow-elevation prompt). + #[cfg_attr( + not(windows), + expect( + dead_code, + reason = "read only by the Windows in-flow-elevation prompt" + ) + )] + pub(crate) action: &'static str, + /// The command to re-run elevated — `"uffs --uninstall"` / `"uffs + /// --update"`. pub(crate) rerun_cmd: &'static str, } @@ -43,6 +57,15 @@ pub(crate) struct GateWording { pub(crate) enum ElevationChoice { /// Elevated, dry-run, or nothing needs Administrator — plan untouched. NotNeeded, + /// Windows in-flow elevation: keep the admin items; the mutating step + /// routes them through a one-shot elevated helper (a single UAC + /// prompt). Produced only when the caller offers in-flow elevation AND + /// on Windows. + #[cfg_attr( + not(windows), + expect(dead_code, reason = "constructed only on the Windows in-flow-UAC path") + )] + ElevateLater, /// Continuing without the admin items: they were dropped from the plan; /// carries their descriptions for the final summary. ContinueWithout(Vec), @@ -51,7 +74,9 @@ pub(crate) enum ElevationChoice { /// The elevation gate — the first question, before any mutation. Returns /// [`ElevationChoice::NotNeeded`] when elevated, under `dry_run`, or when /// nothing needs Administrator. `assume_yes` continues without asking (a -/// scripted run must never block on a prompt it cannot answer). +/// scripted run must never block on a prompt it cannot answer, nor trigger a +/// surprise UAC). `offer_inflow_elevation` enables the Windows 3-way choice for +/// flows that can elevate in place. /// /// # Errors /// @@ -60,6 +85,7 @@ pub(crate) fn elevation_gate( plan: &mut impl ElevatablePlan, dry_run: bool, assume_yes: bool, + offer_inflow_elevation: bool, wording: &GateWording, ) -> Result { if dry_run || !plan.requires_elevation() || uffs_mft::platform::is_elevated() { @@ -71,6 +97,52 @@ pub(crate) fn elevation_gate( plan.drop_elevation_required(), )); } + platform_elevation_choice(plan, offer_inflow_elevation, wording) +} + +/// Windows: the 3-way choice when the flow can elevate in place (`e` records +/// the decision — the single UAC prompt appears later, when the mutating step +/// runs); otherwise the binary continue-without / abort. +#[cfg(windows)] +fn platform_elevation_choice( + plan: &mut impl ElevatablePlan, + offer_inflow_elevation: bool, + wording: &GateWording, +) -> Result { + if !offer_inflow_elevation { + return binary_choice(plan, wording); + } + let choice = prompt_line(&format!( + "\n e = elevate at {} time (Windows shows one UAC prompt)\n\ + \x20 c = continue without it (the item(s) above are left as-is)\n\ + \x20 a = abort\n\n\ + Choice [e/c/A]: ", + wording.action, + ))?; + match choice.as_str() { + "e" | "elevate" => Ok(ElevationChoice::ElevateLater), + "c" | "continue" => Ok(ElevationChoice::ContinueWithout( + plan.drop_elevation_required(), + )), + _ => bail!( + "aborted — re-run `{}` from an elevated (Administrator) terminal to include the item(s) above", + wording.rerun_cmd + ), + } +} + +/// Non-Windows: there is no UAC to request, so the choice is always binary. +#[cfg(not(windows))] +fn platform_elevation_choice( + plan: &mut impl ElevatablePlan, + _offer_inflow_elevation: bool, + wording: &GateWording, +) -> Result { + binary_choice(plan, wording) +} + +/// The continue-without / abort choice (no in-flow elevation available). +fn binary_choice(plan: &mut impl ElevatablePlan, wording: &GateWording) -> Result { let choice = prompt_line( "\n c = continue without it (the item(s) above are left as-is)\n\ \x20 a = abort\n\n\ diff --git a/crates/uffs-cli/src/commands/uninstall/mod.rs b/crates/uffs-cli/src/commands/uninstall/mod.rs index 436847109..9e54bfa89 100644 --- a/crates/uffs-cli/src/commands/uninstall/mod.rs +++ b/crates/uffs-cli/src/commands/uninstall/mod.rs @@ -34,6 +34,8 @@ use anyhow::{Context as _, Result, bail}; use args::UninstallArgs; use plan::{PlanTarget, RemovalPlan}; +use crate::commands::elevation::{self, ElevatablePlan as _, ElevationChoice}; + /// Entry point for `uffs --uninstall`. `args` is every token after the /// `--uninstall` command token. /// @@ -90,7 +92,7 @@ pub(crate) fn run_uninstall(args: &[String]) -> Result<()> { let gate = elevation_gate(&parsed, &mut removal_plan)?; let skipped_elevation: Vec = match &gate { ElevationChoice::ContinueWithout(items) => items.clone(), - ElevationChoice::NotNeeded | ElevationChoice::ElevateAtRemoval => Vec::new(), + ElevationChoice::NotNeeded | ElevationChoice::ElevateLater => Vec::new(), }; // Wait for the gather (spinner) / run it now (`-v`), then present the @@ -121,7 +123,7 @@ pub(crate) fn run_uninstall(args: &[String]) -> Result<()> { render::print_extra_table(&gathered.strays); render::print_plan(&removal_plan, stray_plan); render::print_skipped_elevation(&skipped_elevation); - if matches!(gate, ElevationChoice::ElevateAtRemoval) { + if matches!(gate, ElevationChoice::ElevateLater) { render::print_uac_note(); } @@ -161,7 +163,7 @@ pub(crate) fn run_uninstall(args: &[String]) -> Result<()> { &removal_plan, stray_plan, remove_strays, - matches!(gate, ElevationChoice::ElevateAtRemoval), + matches!(gate, ElevationChoice::ElevateLater), broker_remains, ); Ok(()) @@ -285,88 +287,27 @@ fn binary_dir_bytes(dir: &std::path::Path, stems: &[String]) -> u64 { .fold(0, u64::saturating_add) } -/// What the elevation gate decided for this run. -enum ElevationChoice { - /// Elevated, `--dry-run`, or nothing needs Administrator — plan untouched. - NotNeeded, - /// Windows, non-elevated: keep the admin items in the plan; removal routes - /// them through a one-shot elevated helper (a single UAC prompt at removal - /// time — see [`effects`]). - #[cfg_attr( - not(windows), - expect(dead_code, reason = "constructed only on the Windows UAC path") - )] - ElevateAtRemoval, - /// Non-elevated, continuing without the admin items: they are dropped from - /// the plan; carries their descriptions for the final summary's "NOT - /// removed in this run" note. - ContinueWithout(Vec), -} - /// M3 elevation gate (U-30): THE FIRST question, before any analysis output. -/// The broker (its `LocalSystem` service) is the only admin-only part; a -/// non-elevated run is told immediately what needs Administrator and decides -/// once — elevate at removal time (Windows: one UAC prompt), continue without -/// (items dropped so the final summary never lists work that will not happen), -/// or abort. Skipped when elevated, under `--dry-run` (the preview keeps the -/// "needs Administrator" markers and notes that a real run asks), or when -/// nothing needs Administrator. `--yes` continues without asking — a scripted -/// run must never trigger a surprise UAC prompt. -/// `uffs_mft::platform::is_elevated` is cross-platform (Windows token check; -/// Unix effective-uid 0). +/// Delegates to the shared [`elevation`] gate. The broker (its `LocalSystem` +/// service) is the only admin-only part, and uninstall CAN elevate **in place** +/// — a one-shot elevated helper at removal time (one UAC prompt) — so it offers +/// the Windows 3-way choice (`offer_inflow_elevation = true`). Skipped when +/// elevated, under `--dry-run`, or when nothing needs Administrator; `--yes` +/// continues without asking (a scripted run never triggers a surprise UAC). fn elevation_gate( parsed: &UninstallArgs, removal_plan: &mut RemovalPlan, ) -> Result { - if parsed.dry_run || !removal_plan.requires_elevation() || uffs_mft::platform::is_elevated() { - return Ok(ElevationChoice::NotNeeded); - } - render::print_elevation_gate(removal_plan); - if parsed.assume_yes { - return Ok(ElevationChoice::ContinueWithout( - removal_plan.drop_elevation_required(), - )); - } - platform_elevation_choice(removal_plan) -} - -/// Windows: the interactive 3-way elevation choice. `e` records the decision — -/// the single UAC prompt appears later, when removal actually starts, so -/// nothing is elevated before the final confirmation. -#[cfg(windows)] -fn platform_elevation_choice(removal_plan: &mut RemovalPlan) -> Result { - let choice = prompt_choice( - "\n e = elevate at removal time (Windows shows one UAC prompt)\n\ - \x20 c = continue without it (the item(s) above stay installed)\n\ - \x20 a = abort\n\ - \n\ - Choice [e/c/A]: ", - )?; - match choice.as_str() { - "e" | "elevate" => Ok(ElevationChoice::ElevateAtRemoval), - "c" | "continue" => Ok(ElevationChoice::ContinueWithout( - removal_plan.drop_elevation_required(), - )), - _ => bail!( - "aborted — re-run `uffs --uninstall` from an elevated (Administrator) terminal to remove everything" - ), - } -} - -/// Non-Windows: there is no UAC to request, so the choice stays binary — -/// continue without the elevation-required items, or abort to re-run elevated. -#[cfg(not(windows))] -fn platform_elevation_choice(removal_plan: &mut RemovalPlan) -> Result { - if confirm( - "\nContinue without elevation? Everything else is still uninstalled; the\n\ - item(s) above are left in place. (No aborts so you can re-run elevated) [y/N] ", - )? { - Ok(ElevationChoice::ContinueWithout( - removal_plan.drop_elevation_required(), - )) - } else { - bail!("aborted — re-run `uffs --uninstall` elevated (sudo) to remove everything") - } + elevation::elevation_gate( + removal_plan, + parsed.dry_run, + parsed.assume_yes, + true, + &elevation::GateWording { + action: "removal", + rerun_cmd: "uffs --uninstall", + }, + ) } /// Read one line of input for a multi-choice prompt, trimmed and lowercased. diff --git a/crates/uffs-cli/src/commands/uninstall/plan.rs b/crates/uffs-cli/src/commands/uninstall/plan.rs index b0fb2bd06..2e22faa99 100644 --- a/crates/uffs-cli/src/commands/uninstall/plan.rs +++ b/crates/uffs-cli/src/commands/uninstall/plan.rs @@ -19,6 +19,7 @@ use super::args::{UninstallArgs, UninstallScope}; use super::inventory::{ArtifactKind, BrokerServiceState, Inventory}; #[cfg(windows)] use super::sweep::StrayHit; +use crate::commands::elevation::ElevatablePlan; use crate::commands::update::model::{Channel, Component, DetectionReport, InstallRoot, Scope}; /// The `WinGet` package id UFFS publishes under. @@ -176,21 +177,9 @@ pub(crate) struct RemovalPlan { pub(crate) groups: Vec, } -impl RemovalPlan { - /// Iterate every item across all groups, in order. - pub(crate) fn items(&self) -> impl Iterator { - self.groups.iter().flat_map(|group| &group.items) - } - - /// Total bytes the plan would reclaim. - pub(crate) fn total_bytes(&self) -> u64 { - self.items() - .map(|item| item.bytes) - .fold(0, u64::saturating_add) - } - +impl ElevatablePlan for RemovalPlan { /// Whether any item requires Administrator. - pub(crate) fn requires_elevation(&self) -> bool { + fn requires_elevation(&self) -> bool { self.items().any(|item| item.needs_elevation) } @@ -199,7 +188,7 @@ impl RemovalPlan { /// everything it *can* and leave the broker for an elevated re-run. Returns /// the dropped items' descriptions so the final summary can list exactly /// what this run skips. - pub(crate) fn drop_elevation_required(&mut self) -> Vec { + fn drop_elevation_required(&mut self) -> Vec { let mut dropped: Vec = Vec::new(); for group in &mut self.groups { group.items.retain(|item| { @@ -214,6 +203,25 @@ impl RemovalPlan { dropped } + /// The non-elevated preamble listing the admin-only items (the broker). + fn render_elevation_needed(&self) { + super::render::print_elevation_gate(self); + } +} + +impl RemovalPlan { + /// Iterate every item across all groups, in order. + pub(crate) fn items(&self) -> impl Iterator { + self.groups.iter().flat_map(|group| &group.items) + } + + /// Total bytes the plan would reclaim. + pub(crate) fn total_bytes(&self) -> u64 { + self.items() + .map(|item| item.bytes) + .fold(0, u64::saturating_add) + } + /// Fill in the reclaim bytes of every binary-delete item, so the summary's /// "Reclaims ~N" reflects the binaries too (not just the data dirs). /// Statting files is IO, which this pure module leaves to the caller: diff --git a/crates/uffs-cli/src/commands/uninstall/plan/tests.rs b/crates/uffs-cli/src/commands/uninstall/plan/tests.rs index 1c1881838..8a65683cb 100644 --- a/crates/uffs-cli/src/commands/uninstall/plan/tests.rs +++ b/crates/uffs-cli/src/commands/uninstall/plan/tests.rs @@ -10,6 +10,7 @@ use std::path::PathBuf; #[cfg(windows)] use super::build_stray_plan; use super::{PlanTarget, RemovalPlan, build_plan}; +use crate::commands::elevation::ElevatablePlan as _; use crate::commands::uninstall::args::{UninstallArgs, UninstallScope}; use crate::commands::uninstall::inventory::{ ArtifactDir, ArtifactKind, BrokerServiceState, Inventory, diff --git a/crates/uffs-cli/src/commands/uninstall/render.rs b/crates/uffs-cli/src/commands/uninstall/render.rs index 39e5b5884..e6e00b921 100644 --- a/crates/uffs-cli/src/commands/uninstall/render.rs +++ b/crates/uffs-cli/src/commands/uninstall/render.rs @@ -15,6 +15,7 @@ use super::remove::{ItemStatus, RemovalOutcome}; use super::resolve_order::{ResolutionState, StemResolution}; #[cfg(windows)] use super::sweep::StrayHit; +use crate::commands::elevation::ElevatablePlan as _; use crate::commands::update::model::{Channel, Scope}; /// Print the running build's version + git commit at the top of an uninstall diff --git a/crates/uffs-cli/src/commands/update/mod.rs b/crates/uffs-cli/src/commands/update/mod.rs index 36d351433..df6349c94 100644 --- a/crates/uffs-cli/src/commands/update/mod.rs +++ b/crates/uffs-cli/src/commands/update/mod.rs @@ -315,8 +315,12 @@ fn run_automatic_update(report: &DetectionReport, verbose: bool) -> Result<()> { // (continue-without / abort) — so nothing fails mid-swap. The per-root // elevation model lives in `plan`; the gate in `commands::elevation`. let mut plan = plan::UpdatePlan::build(report, &latest); + // Update has no in-flow elevation (winget refuses a user package elevated), + // so `offer_inflow_elevation = false` → the gate is the binary + // continue-without / abort. if let elevation::ElevationChoice::ContinueWithout(dropped) = - elevation::elevation_gate(&mut plan, false, false, &elevation::GateWording { + elevation::elevation_gate(&mut plan, false, false, false, &elevation::GateWording { + action: "update", rerun_cmd: "uffs --update", })? { From f551d359f7d25a0594d2a4a41a875cba20293cfd Mon Sep 17 00:00:00 2001 From: Robert M1 <50460704+githubrobbi@users.noreply.github.com> Date: Wed, 8 Jul 2026 14:48:22 -0700 Subject: [PATCH 2/7] fix(uninstall): deep-sweep daemon reload stops cooperatively, not gated kill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deep-sweep (`d`) reload shelled the policy-gated `daemon kill` to stop the running daemon before restarting it with full drive coverage. A non-elevated shell can't use that on an elevated daemon, so it failed loudly ("requires an elevated shell"), bailed before the elevated start, and the UAC prompt the `d` prompt promised never appeared. Stop the daemon COOPERATIVELY first — an IPC shutdown the daemon honors regardless of its own elevation (same-user pipe), the same stop `winget::quiesce` uses — and fall back to the gated kill only if the daemon ignores it. Then the reload reaches the elevated start and delivers the promised UAC. - New `stop_running_daemon` (cooperative shutdown -> poll down -> gated-kill fallback -> re-check) + `daemon_is_down`; `wait_until_daemon_down` shares the down-check. - Narration "kill + start" -> "stop + start". xwin (compiles this #[cfg(windows)] path) + native clippy + rustdoc clean; 147 uffs-cli tests pass. --- .../src/commands/uninstall/coverage.rs | 46 +++++++++++++++---- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/crates/uffs-cli/src/commands/uninstall/coverage.rs b/crates/uffs-cli/src/commands/uninstall/coverage.rs index 2e8f07eb0..b1f01cd8c 100644 --- a/crates/uffs-cli/src/commands/uninstall/coverage.rs +++ b/crates/uffs-cli/src/commands/uninstall/coverage.rs @@ -165,23 +165,26 @@ fn reload_daemon_for_coverage( notes, format!( "\nThe daemon needs a reload before the deep sweep ({list}).\n\ - Reloading it (kill + start):" + Reloading it (stop + start):" ), ); } - if let Err(err) = run_handler(quiet, &DaemonAction::Kill) { + // Stop the running daemon COOPERATIVELY (an IPC shutdown it honors + // regardless of its own elevation — the same-user pipe), NOT the + // policy-gated `daemon kill`, which a non-elevated shell cannot use on an + // elevated daemon (the loud failure the user hit). Only if the daemon cannot + // be stopped at all do we fall back to scanning what is already indexed. + if !stop_running_daemon(quiet) { emit( quiet, notes, - format!( - "\nNote: could not stop the running daemon ({err}).\n\ - The deep sweep will scan the drives already indexed." - ), + "\nNote: could not stop the running daemon.\n\ + The deep sweep will scan the drives already indexed." + .to_owned(), ); return; } - wait_until_daemon_down(); if let Err(err) = run_handler(quiet, &start_action(elevate_daemon)) { emit(quiet, notes, start_failure_note(elevate_daemon, &err)); @@ -264,12 +267,39 @@ fn start_action(elevate: bool) -> DaemonAction { } } +/// Stop the running daemon for the reload. Tries the **cooperative** IPC +/// shutdown first — the daemon exits itself on the RPC regardless of its own +/// elevation (the pipe is same-user), so it works from a non-elevated shell, +/// unlike the policy-gated `daemon kill`. Falls back to the gated kill only if +/// the daemon ignores the shutdown (works for a non-elevated / broker daemon; +/// needs Administrator for an elevated one). Returns whether the daemon is +/// actually down afterward. +fn stop_running_daemon(quiet: bool) -> bool { + if UffsClientSync::connect_raw().is_ok_and(|mut client| client.shutdown().is_ok()) { + wait_until_daemon_down(); + } + if daemon_is_down() { + return true; + } + // The daemon ignored the cooperative shutdown (or its pipe was unreachable + // yet the process lives) — try the gated kill, then re-check. + if run_handler(quiet, &DaemonAction::Kill).is_ok() { + wait_until_daemon_down(); + } + daemon_is_down() +} + +/// Whether the daemon's IPC endpoint is no longer reachable (fully shut down). +fn daemon_is_down() -> bool { + UffsClientSync::connect_raw().is_err() +} + /// Poll until the daemon is no longer reachable (fully shut down) or /// [`SHUTDOWN_WAIT`] elapses. fn wait_until_daemon_down() { let deadline = Instant::now() + SHUTDOWN_WAIT; while Instant::now() < deadline { - if UffsClientSync::connect_raw().is_err() { + if daemon_is_down() { return; } std::thread::sleep(POLL_INTERVAL); From 6b8be63a8c43eb2d43c09456c59441f4e26d50ae Mon Sep 17 00:00:00 2001 From: Robert M1 <50460704+githubrobbi@users.noreply.github.com> Date: Wed, 8 Jul 2026 14:58:28 -0700 Subject: [PATCH 3/7] fix(broker): --uninstall is a no-op when the service is already absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `uffs-broker --uninstall` bailed on any `sc delete` failure, so removing a broker that was never installed screamed "[SC] OpenService FAILED 1060: The specified service does not exist" — even though "not installed" is exactly the requested end state. Treat ERROR_SERVICE_DOES_NOT_EXIST (1060) as success: the request is satisfied by doing nothing. New pure `absent_service_signal` classifier (exit code primary, sc text fallback) + Windows unit tests. native + windows-msvc clippy + rustdoc clean. --- crates/uffs-broker/src/broker/service.rs | 61 ++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 5 deletions(-) diff --git a/crates/uffs-broker/src/broker/service.rs b/crates/uffs-broker/src/broker/service.rs index f7c03d862..c27fb8079 100644 --- a/crates/uffs-broker/src/broker/service.rs +++ b/crates/uffs-broker/src/broker/service.rs @@ -213,12 +213,37 @@ pub(super) fn uninstall_service() -> anyhow::Result<()> { if output.status.success() { println!("Service uninstalled."); - } else { - // AUDIT-OK(bytes): `sc` command output surfaced verbatim in an error - // message for the operator — display only, no decision. - anyhow::bail!("Uninstall failed: {}", sc_output(&output)); + return Ok(()); } - Ok(()) + + // The service already not existing IS the requested end state, so treat it + // as a no-op success rather than a loud failure. `sc delete` on a missing + // service returns ERROR_SERVICE_DOES_NOT_EXIST (1060). + if service_already_absent(&output) { + println!("Broker service is not installed — nothing to remove."); + return Ok(()); + } + + // AUDIT-OK(bytes): `sc` command output surfaced verbatim in an error + // message for the operator — display only, no decision. + anyhow::bail!("Uninstall failed: {}", sc_output(&output)); +} + +/// Whether an `sc delete` failure is just "the service does not exist" +/// (`ERROR_SERVICE_DOES_NOT_EXIST`, 1060) — i.e. already uninstalled, so the +/// uninstall request is already satisfied. +fn service_already_absent(output: &std::process::Output) -> bool { + // AUDIT-OK(bytes): the `sc` output is inspected only to classify the "already + // gone" case; the exit code is the primary signal, the text a fallback. + absent_service_signal(output.status.code(), &sc_output(output)) +} + +/// Pure classifier: `sc delete` reports a missing service as +/// `ERROR_SERVICE_DOES_NOT_EXIST` (1060) — via the process exit code (primary) +/// or its text (fallback). Split out so the "already gone → success" decision +/// is unit-testable without spawning `sc`. +fn absent_service_signal(exit_code: Option, sc_text: &str) -> bool { + exit_code == Some(1060) || sc_text.contains("1060") } // ── FU-1: Windows Service control dispatcher ──────────────────────────────── @@ -385,3 +410,29 @@ fn signal_stop() { } } } + +#[cfg(test)] +mod tests { + use super::absent_service_signal; + + #[test] + fn absent_when_sc_reports_1060_by_code_or_text() { + // Exit code is the primary signal. + assert!(absent_service_signal(Some(1060_i32), "")); + // Text is the fallback when the code is not surfaced. + assert!(absent_service_signal( + None, + "[SC] OpenService FAILED 1060:\n\nThe specified service does not exist" + )); + } + + #[test] + fn other_failures_are_not_treated_as_absent() { + // Access denied (5) is a real failure, not "already gone". + assert!(!absent_service_signal( + Some(5_i32), + "[SC] DeleteService FAILED 5:\n\nAccess is denied." + )); + assert!(!absent_service_signal(None, "some unrelated error")); + } +} From 257a45f1f0d8ee4e19270d27f4ae46404ef4d493 Mon Sep 17 00:00:00 2001 From: Robert M1 <50460704+githubrobbi@users.noreply.github.com> Date: Wed, 8 Jul 2026 16:23:27 -0700 Subject: [PATCH 4/7] feat(uninstall): spinner during the deep-sweep daemon reload; share the spinner The deep-sweep (`d`) reload starts the daemon, which on a cold cache can take ~90s. In loud mode that showed only raw "connect attempt N/20" chatter, which reads as a hang. Animate a spinner over the (quiet) start instead so the wait reads as "working". Extract the spinner into a shared `commands::spinner` (Windows-only for now, since both callers are `#[cfg(windows)]`) and point both the winget orchestration and the new coverage reload at it, removing winget's private copy (behavior-identical output). native + windows-msvc clippy + rustdoc clean; 147 uffs-cli tests pass. --- crates/uffs-cli/src/commands.rs | 6 +++ crates/uffs-cli/src/commands/spinner.rs | 40 +++++++++++++++++++ .../src/commands/uninstall/coverage.rs | 17 +++++++- crates/uffs-cli/src/commands/update/winget.rs | 23 +---------- 4 files changed, 63 insertions(+), 23 deletions(-) create mode 100644 crates/uffs-cli/src/commands/spinner.rs diff --git a/crates/uffs-cli/src/commands.rs b/crates/uffs-cli/src/commands.rs index 0bc69ba3b..8f00d7284 100644 --- a/crates/uffs-cli/src/commands.rs +++ b/crates/uffs-cli/src/commands.rs @@ -34,6 +34,12 @@ pub(crate) mod mcp_mgmt; pub mod output; /// Search command implementation. pub mod search; +/// Braille spinner for blocking steps with no incremental progress of their own +/// (winget upgrade, a cold-cache daemon start). Windows-only for now — its only +/// callers (winget orchestration, uninstall coverage reload) are +/// `#[cfg(windows)]`. +#[cfg(windows)] +pub(crate) mod spinner; /// Stats subcommand implementation. pub mod stats; /// Combined `uffs --status` command. diff --git a/crates/uffs-cli/src/commands/spinner.rs b/crates/uffs-cli/src/commands/spinner.rs new file mode 100644 index 000000000..cb1224162 --- /dev/null +++ b/crates/uffs-cli/src/commands/spinner.rs @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2025-2026 SKY, LLC. + +//! A braille spinner for blocking CLI steps that have no incremental progress +//! of their own (a `winget upgrade`, a cold-cache daemon start, …). Runs the +//! work on a scoped thread and animates until it finishes, then erases the +//! line — so a ~90 s wait reads as "working", not "hung". + +use core::time::Duration; +use std::io::Write as _; + +/// Spinner frame interval. +const POLL: Duration = Duration::from_millis(120); + +/// Braille spinner frames. +const FRAMES: [&str; 10] = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; + +/// Run `body` on a scoped thread while animating a spinner after `label`, then +/// erase the spinner line so the caller's next output lands clean. Returns +/// whatever `body` returns. For a blocking call that prints nothing itself — +/// drive the underlying work in its *quiet* form so the spinner owns the line. +#[expect(clippy::print_stdout, reason = "interactive progress spinner")] +pub(crate) fn spinner_while(label: &str, body: impl FnOnce() -> T + Send) -> T { + std::thread::scope(|scope| { + let handle = scope.spawn(body); + let mut frame = 0_usize; + while !handle.is_finished() { + let glyph = FRAMES.get(frame % FRAMES.len()).copied().unwrap_or("*"); + print!("\r {glyph} {label}\u{2026} "); + let _flushed = std::io::stdout().flush(); + std::thread::sleep(POLL); + frame = frame.wrapping_add(1); + } + print!("\r{:60}\r", ""); + let _flushed = std::io::stdout().flush(); + // Our closures never panic; aborting is safer than an unwrap the + // workspace lints forbid anyway. + handle.join().unwrap_or_else(|_| std::process::abort()) + }) +} diff --git a/crates/uffs-cli/src/commands/uninstall/coverage.rs b/crates/uffs-cli/src/commands/uninstall/coverage.rs index b1f01cd8c..70cdc9292 100644 --- a/crates/uffs-cli/src/commands/uninstall/coverage.rs +++ b/crates/uffs-cli/src/commands/uninstall/coverage.rs @@ -186,7 +186,7 @@ fn reload_daemon_for_coverage( return; } - if let Err(err) = run_handler(quiet, &start_action(elevate_daemon)) { + if let Err(err) = start_daemon_for_coverage(quiet, elevate_daemon) { emit(quiet, notes, start_failure_note(elevate_daemon, &err)); return; } @@ -240,6 +240,21 @@ fn run_handler(quiet: bool, action: &DaemonAction) -> anyhow::Result<()> { } } +/// Start the (possibly elevated) daemon for the reload. A cold cache can take +/// ~90 s to load; in loud mode animate a spinner over the *quiet* start (the +/// raw "connect attempt N/20" chatter reads as a hang), so the spinner owns the +/// line. Quiet mode stays silent — its narration is deferred as a note by the +/// caller. +fn start_daemon_for_coverage(quiet: bool, elevate_daemon: bool) -> anyhow::Result<()> { + if quiet { + return daemon_mgmt::daemon_quiet(&start_action(elevate_daemon)); + } + crate::commands::spinner::spinner_while( + "starting the index daemon (a cold cache can take up to ~90 s)", + || daemon_mgmt::daemon_quiet(&start_action(elevate_daemon)), + ) +} + /// Route one narration line: printed live in loud mode, deferred as a note in /// quiet mode (the caller prints notes with the final presentation). #[expect(clippy::print_stdout, reason = "CLI progress output (loud mode only)")] diff --git a/crates/uffs-cli/src/commands/update/winget.rs b/crates/uffs-cli/src/commands/update/winget.rs index 0fe933b3d..ea53d4b95 100644 --- a/crates/uffs-cli/src/commands/update/winget.rs +++ b/crates/uffs-cli/src/commands/update/winget.rs @@ -168,6 +168,7 @@ mod windows_impl { use anyhow::{Context as _, Result, bail}; use super::{UpgradeOutcome, WINGET_PACKAGE_ID}; + use crate::commands::spinner::spinner_while; use crate::commands::update::model::{Channel, DetectionReport, InstallRoot, Scope}; use crate::commands::update::strip_verbatim_prefix; @@ -585,28 +586,6 @@ mod windows_impl { } } - /// Run `body` on a scoped thread while animating a spinner (for a blocking - /// call with no incremental progress, e.g. `winget upgrade`). - #[expect(clippy::print_stdout, reason = "interactive progress spinner")] - fn spinner_while(label: &str, body: impl FnOnce() -> T + Send) -> T { - const FRAMES: [&str; 10] = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; - std::thread::scope(|scope| { - let handle = scope.spawn(body); - let mut frame = 0_usize; - while !handle.is_finished() { - let glyph = FRAMES.get(frame % FRAMES.len()).copied().unwrap_or("*"); - print!("\r {glyph} {label}\u{2026} "); - let _flushed = std::io::stdout().flush(); - std::thread::sleep(POLL); - frame = frame.wrapping_add(1); - } - clear_line(); - // Our closures never panic; if one somehow did, aborting is safer - // than an unwrap that the workspace lints forbid anyway. - handle.join().unwrap_or_else(|_| std::process::abort()) - }) - } - /// Erase the spinner line. #[expect(clippy::print_stdout, reason = "interactive progress spinner")] fn clear_line() { From f424672cd0cd4e8247e0669fe239e7178ccb9047 Mon Sep 17 00:00:00 2001 From: Robert M1 <50460704+githubrobbi@users.noreply.github.com> Date: Wed, 8 Jul 2026 16:33:31 -0700 Subject: [PATCH 5/7] fix(broker): --uninstall no-ops without elevation when no service exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the 1060 no-op: the existence check now runs BEFORE the Administrator gate. A non-elevated `uffs-broker --uninstall` with no broker installed prints "Broker service is not installed — nothing to remove." and exits 0, instead of demanding an elevated terminal for work that would not happen. Elevation is still required only when there is actually a service to delete. Existence is a non-elevated SCM query (`uffs_winsvc::is_installed`). native + windows-msvc clippy clean. --- crates/uffs-broker/src/broker/service.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/uffs-broker/src/broker/service.rs b/crates/uffs-broker/src/broker/service.rs index c27fb8079..a453ca223 100644 --- a/crates/uffs-broker/src/broker/service.rs +++ b/crates/uffs-broker/src/broker/service.rs @@ -193,6 +193,13 @@ pub(super) fn install_service() -> anyhow::Result<()> { reason = "CLI admin command — stdout is the user-visible result channel" )] pub(super) fn uninstall_service() -> anyhow::Result<()> { + // Checking existence is a non-elevated SCM query. If the service is already + // absent, the requested end state holds — a no-op success, and there is no + // reason to demand Administrator for work that would not happen. + if !uffs_winsvc::is_installed(SERVICE_NAME) { + println!("Broker service is not installed — nothing to remove."); + return Ok(()); + } if !super::is_elevated() { anyhow::bail!( "removing the broker service requires Administrator.\n\ From adb6b880e0a93a469369ddd3007bf7679a190dca Mon Sep 17 00:00:00 2001 From: Robert M1 <50460704+githubrobbi@users.noreply.github.com> Date: Wed, 8 Jul 2026 17:05:06 -0700 Subject: [PATCH 6/7] fix(spinner): erase the full drawn line, not a fixed 60 columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A label longer than 60 chars (the deep-sweep "starting the index daemon (a cold cache can take up to ~90 s)") left a stale tail ("90 s)…") on screen after the spinner finished, because the erase wiped a fixed 60 columns. Clear `label.chars().count() + 11` columns — exactly the "