From 7bae353320e8b462ecadb18fbfd10d9f3013f56b Mon Sep 17 00:00:00 2001 From: zackees Date: Fri, 7 Aug 2026 06:25:36 -0700 Subject: [PATCH] feat(port): add `doctor --fix` for USB selective suspend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the remedy half of #1279. Disables USB selective suspend, which is the one thing on the host that can manufacture the phantom this command exists to explain: Windows powers a port down, a board fails to resume, and it returns as code 43 whose stale COM record then reads `health=phantom`. fbuild port doctor --dry-run # plan only, never elevates fbuild port doctor --fix --yes # one UAC prompt for the whole set Guards, each deliberate: - `--dry-run` prints the exact argv and never elevates, so it is safe to run blind. - `--fix` alone refuses and points at `--yes`. A host-wide power-policy change must never be a side effect of running a diagnostic. - `--no-elevate` fails cleanly instead of prompting, for CI/headless. - Already-disabled is a no-op that neither prompts nor errors — no UAC for a change with nothing to do. - Both AC and DC indices are set; disabling only AC leaves a laptop suspending ports the moment it is unplugged. - The plan states how to revert. What `--fix` deliberately does NOT do: disable/enable devnodes, or restart hubs. A bus reset is not a VBUS cycle and does not recover a descriptor-failed device, and a root-hub restart disrupts every other device on that hub. A remedy that looks helpful and is not is worse than none. Verified: `--dry-run` prints the plan and exits 0; `--fix` without `--yes` exits 1; `--fix --yes --no-elevate` exits 1. The command construction and plan rendering are pure functions with tests, including that both power indices are covered and that the idempotent path emits no plan. Not verified: the elevated execution path itself. It needs an interactive UAC prompt, which is not something to trigger from an unattended session, so it has been exercised only up to the point of elevation. Refs #1279 Co-Authored-By: Claude --- crates/fbuild-cli/src/cli/port_doctor.rs | 141 +++++++++++++++++++++++ crates/fbuild-cli/src/cli/port_scan.rs | 27 ++++- 2 files changed, 167 insertions(+), 1 deletion(-) diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index c5691b6e..920ba3b0 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -210,6 +210,114 @@ pub fn render_report(diagnoses: &[PortDiagnosis], problems: &[UsbProblemDevice]) out } +/// The exact `powercfg` argv `--fix` would run, AC and DC. +/// +/// Pure so the change set can be shown by `--dry-run` and asserted in tests +/// without touching the host. Both indices matter: disabling only the AC side +/// leaves a laptop suspending ports the moment it is unplugged. +pub fn suspend_fix_commands() -> Vec> { + ["-setacvalueindex", "-setdcvalueindex"] + .iter() + .map(|verb| { + vec![ + "powercfg".to_string(), + (*verb).to_string(), + "SCHEME_CURRENT".to_string(), + USB_SUBGROUP_GUID.to_string(), + SELECTIVE_SUSPEND_GUID.to_string(), + "0".to_string(), + ] + }) + .collect() +} + +/// Human-readable plan for `--fix`, used by `--dry-run` and before elevating. +pub fn render_fix_plan(commands: &[Vec], already_disabled: bool) -> String { + if already_disabled { + // Idempotent: nothing to do, and in particular no UAC prompt for a + // change that would be a no-op. + return "nothing to do — USB selective suspend is already disabled\n".to_string(); + } + let mut out = String::from("would run, elevated:\n"); + for cmd in commands { + out.push_str(" "); + out.push_str(&cmd.join(" ")); + out.push('\n'); + } + out.push_str( + "this changes a host-wide power setting; revert with the same commands and a \ + trailing 1 instead of 0\n", + ); + out +} + +/// Apply the safe subset of remedies: disable USB selective suspend. +/// +/// Deliberately narrow. It does **not** disable/enable devnodes or restart +/// hubs: a bus reset is not a VBUS cycle and does not recover a +/// descriptor-failed device, while a root-hub restart disrupts every other +/// device on that hub. A fix that looks helpful and is not is worse than none. +pub fn run_fix(dry_run: bool, assume_yes: bool, no_elevate: bool) -> Result<()> { + let current = query_selective_suspend(); + let already_disabled = current == Some(false); + let commands = suspend_fix_commands(); + crate::output::result(render_fix_plan(&commands, already_disabled).trim_end_matches('\n')); + + if already_disabled || dry_run { + return Ok(()); + } + if !cfg!(windows) { + crate::output::result("not applicable on this platform"); + return Ok(()); + } + if no_elevate { + return Err(FbuildError::SerialError( + "--no-elevate was passed but this change needs administrator rights; \ + re-run without it, or run the commands above from an elevated shell" + .to_string(), + )); + } + if !assume_yes { + // A host-wide power-policy change should never be a side effect of a + // diagnostic. Require the caller to say so. + return Err(FbuildError::SerialError( + "this changes a host-wide power setting; re-run with --yes to apply, \ + or --dry-run to see the plan only" + .to_string(), + )); + } + + // One elevation for the whole change set, not one per command. + let joined = commands + .iter() + .map(|c| c.join(" ")) + .collect::>() + .join("; "); + let script = + format!("Start-Process -Verb RunAs -Wait -FilePath cmd -ArgumentList '/c {joined}'"); + let out = fbuild_core::subprocess::run_command_blocking( + &[ + "powershell", + "-NoProfile", + "-NonInteractive", + "-Command", + &script, + ], + None, + None, + Some(std::time::Duration::from_secs(120)), + ) + .map_err(|e| FbuildError::SerialError(format!("elevation failed: {e}")))?; + if !out.success() { + return Err(FbuildError::SerialError(format!( + "elevated powercfg failed: {}", + out.stderr.trim() + ))); + } + crate::output::result("USB selective suspend disabled; re-run `fbuild port doctor` to confirm"); + Ok(()) +} + /// `fbuild port doctor` entry point. pub fn run(only_port: Option<&str>) -> Result<()> { let ports = fbuild_serial::ports::available_ports() @@ -405,4 +513,37 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) assert!(render_suspend_section(None).is_empty()); assert!(render_suspend_section(Some(false)).contains("disabled")); } + + /// Both indices matter: disabling only AC leaves a laptop suspending + /// ports the moment it is unplugged. + #[test] + fn fix_covers_both_ac_and_dc() { + let cmds = suspend_fix_commands(); + assert_eq!(cmds.len(), 2); + assert!(cmds.iter().any(|c| c.contains(&"-setacvalueindex".into()))); + assert!(cmds.iter().any(|c| c.contains(&"-setdcvalueindex".into()))); + for c in &cmds { + assert_eq!(c.last().unwrap(), "0", "must disable, not enable"); + assert!(c.contains(&USB_SUBGROUP_GUID.to_string())); + assert!(c.contains(&SELECTIVE_SUSPEND_GUID.to_string())); + } + } + + /// Idempotence: an already-disabled host must produce no plan, so `--fix` + /// never raises a UAC prompt for a no-op. + #[test] + fn fix_plan_is_empty_when_already_disabled() { + let plan = render_fix_plan(&suspend_fix_commands(), true); + assert!(plan.contains("nothing to do"), "got: {plan}"); + assert!(!plan.contains("would run"), "got: {plan}"); + } + + /// A host-wide change must show exactly what it will run, and how to undo it. + #[test] + fn fix_plan_shows_commands_and_how_to_revert() { + let plan = render_fix_plan(&suspend_fix_commands(), false); + assert!(plan.contains("would run, elevated"), "got: {plan}"); + assert!(plan.contains("-setacvalueindex"), "got: {plan}"); + assert!(plan.contains("revert"), "got: {plan}"); + } } diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index 2b257cef..92279ccc 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -36,6 +36,19 @@ pub enum PortAction { /// Diagnose a single port (e.g. `COM17`). Defaults to every port. #[arg(long)] port: Option, + /// Apply the safe remedies (currently: disable USB selective suspend). + /// Needs administrator rights and `--yes`. + #[arg(long)] + fix: bool, + /// Print exactly what `--fix` would run and change nothing. Never elevates. + #[arg(long)] + dry_run: bool, + /// Confirm a host-wide change. Required by `--fix`. + #[arg(long)] + yes: bool, + /// Fail rather than raising a UAC prompt. For CI and headless runs. + #[arg(long)] + no_elevate: bool, }, } @@ -43,7 +56,19 @@ pub enum PortAction { pub fn run_port(action: PortAction) -> Result<()> { match action { PortAction::Scan { offline } => run_scan(offline), - PortAction::Doctor { port } => super::port_doctor::run(port.as_deref()), + PortAction::Doctor { + port, + fix, + dry_run, + yes, + no_elevate, + } => { + if fix || dry_run { + super::port_doctor::run_fix(dry_run, yes, no_elevate) + } else { + super::port_doctor::run(port.as_deref()) + } + } } }