fix(teensy): address CodeRabbit review on #435

* Two-tier retry timeout: first attempt gets `wait_for_halfkay_timeout_secs`
  (180 s default — covers the "user walks up and presses the program
  button" case); subsequent retries get only `flash_timeout_secs` (30 s
  default) so a wedged device can't burn 15 min of subprocess time before
  the structured diagnostic surfaces. `flash::run_with_retry` takes both
  timeouts explicitly.
* Normalize the caller's explicit port once: `Some("")` becomes `None`
  so an empty string never leaks into `DeploymentResult.port` (which the
  daemon would forward verbatim to the monitor).
* On post-flash port-discovery timeout, fall back to the resolved
  trigger port (the PJRC device we actually flashed) instead of `None`
  so the same-port-after-flash case still produces a usable port name.
* Serialize the three env-var-mutating tests via a shared
  `TEST_ENV_LOCK` so parallel `cargo test` execution can't race the
  global process environment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: zackees

crates/fbuild-deploy/src/teensy/first_byte_probe.rs

@@ -127,6 +127,9 @@ mod tests {
 
     #[test]
     fn env_override_parses_zero() {
+        let _guard = crate::teensy::soft_reboot::TEST_ENV_LOCK
+            .lock()
+            .unwrap_or_else(|e| e.into_inner());
         std::env::set_var("FBUILD_TEENSY_FIRST_BYTE_TIMEOUT_SECS", "0");
         assert_eq!(env_first_byte_timeout_secs_override(), Some(0));
         std::env::set_var("FBUILD_TEENSY_FIRST_BYTE_TIMEOUT_SECS", "30");

crates/fbuild-deploy/src/teensy/flash.rs

@@ -82,12 +82,17 @@ impl FlashRunOutcome {
 /// Run `teensy_loader_cli` up to `retries + 1` times, sleeping `backoff_ms`
 /// between attempts. Stops on the first success.
 ///
-/// `timeout` bounds each individual attempt — not the sum.
+/// Two-tier timeout: the *first* attempt is given `first_attempt_timeout` to
+/// cover the worst-case "user walks up and presses the program button" path
+/// after a baud-134 trigger; every subsequent retry uses the smaller
+/// `subsequent_attempt_timeout` since by then HalfKay has either already been
+/// observed or the device is wedged in a way another retry won't fix.
 pub fn run_with_retry(
     cfg: &FlashConfig,
     retries: u32,
     backoff_ms: u64,
-    timeout: Duration,
+    first_attempt_timeout: Duration,
+    subsequent_attempt_timeout: Duration,
     verbose: bool,
 ) -> Result<FlashRunOutcome> {
     let args: Vec<String> = vec![
@@ -109,7 +114,12 @@ pub fn run_with_retry(
     let mut attempts: Vec<FlashAttempt> = Vec::with_capacity(total_attempts as usize);
 
     for attempt in 1..=total_attempts {
-        let result = run_command(&args_ref, None, None, Some(timeout))?;
+        let attempt_timeout = if attempt == 1 {
+            first_attempt_timeout
+        } else {
+            subsequent_attempt_timeout
+        };
+        let result = run_command(&args_ref, None, None, Some(attempt_timeout))?;
         let success = result.success();
         let exit_code = result.exit_code;
         let stdout = result.stdout;
@@ -225,6 +235,9 @@ mod tests {
 
     #[test]
     fn env_override_parses() {
+        let _guard = crate::teensy::soft_reboot::TEST_ENV_LOCK
+            .lock()
+            .unwrap_or_else(|e| e.into_inner());
         std::env::set_var("FBUILD_TEENSY_FLASH_RETRIES", "9");
         assert_eq!(env_flash_retries_override(), Some(9));
         std::env::set_var("FBUILD_TEENSY_FLASH_RETRIES", "bogus");

crates/fbuild-deploy/src/teensy/mod.rs

@@ -155,7 +155,15 @@ impl Deployer for TeensyDeployer {
     ) -> Result<DeploymentResult> {
         // ---- 1. Pre-flash port snapshot --------------------------------
         let pre_snapshot = port_discovery::snapshot_port_names();
-        let trigger_port = resolve_trigger_port(port);
+        // Normalize the caller-supplied port once: treat `Some("")` as
+        // `None` so we never propagate an empty string into
+        // `DeploymentResult.port` (the daemon would forward it verbatim to
+        // the monitor and produce a `failed to open ""` error).
+        let explicit_port: Option<String> = port
+            .map(str::trim)
+            .filter(|s| !s.is_empty())
+            .map(str::to_string);
+        let trigger_port = resolve_trigger_port(explicit_port.as_deref());
 
         // Best-effort `usb_type` advisory — log it now so the user sees the
         // hint *before* we discover the firmware is silent.
@@ -220,10 +228,15 @@ impl Deployer for TeensyDeployer {
             &flash_cfg,
             self.flash_retries,
             self.flash_retry_backoff_ms,
-            Duration::from_secs(
-                self.wait_for_halfkay_timeout_secs
-                    .max(self.flash_timeout_secs),
-            ),
+            // First attempt may need to wait for the user to press the
+            // program button on a fresh board — full HalfKay budget.
+            Duration::from_secs(self.wait_for_halfkay_timeout_secs),
+            // Subsequent retries: HalfKay was either already observed (the
+            // baud-134 trigger left a fresh window open) or the device is
+            // wedged in a way another retry won't fix — use the tighter
+            // per-flash budget so a wedged board can't burn many minutes
+            // before falling through to the structured diagnostic.
+            Duration::from_secs(self.flash_timeout_secs),
             self.verbose,
         )?;
 
@@ -237,7 +250,7 @@ impl Deployer for TeensyDeployer {
                     attempt_count,
                     flash_outcome.last_exit_code()
                 ),
-                port: port.map(|p| p.to_string()),
+                port: explicit_port.clone(),
                 stdout: flash_outcome.last_stdout().to_string(),
                 stderr: flash_outcome.last_stderr().to_string(),
                 outcome: DeployOutcome::FullFlash,
@@ -251,9 +264,12 @@ impl Deployer for TeensyDeployer {
         ) {
             port_discovery::NewPortOutcome::Found(name) => Some(name),
             port_discovery::NewPortOutcome::TimedOut => {
-                // Re-enumeration may simply have used the same port name — fall
-                // back to the caller-supplied port if any.
-                port.map(|p| p.to_string())
+                // Re-enumeration may have reused the same port name (the
+                // common case on Linux/macOS). Fall back to the explicit
+                // port the caller asked for, and if none was given, to the
+                // PJRC port we triggered against — that's the device the
+                // monitor wants to attach to.
+                explicit_port.clone().or_else(|| trigger_port.clone())
             }
         };
 

crates/fbuild-deploy/src/teensy/soft_reboot.rs

@@ -80,6 +80,12 @@ pub fn baud_134_trigger_disabled() -> bool {
         .unwrap_or(false)
 }
 
+/// Shared mutex used by env-var-mutating tests across this `teensy` module
+/// tree so they cannot race the global process environment when the test
+/// runner schedules them in parallel.
+#[cfg(test)]
+pub(crate) static TEST_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -101,7 +107,11 @@ mod tests {
 
     #[test]
     fn disabled_by_env() {
-        // SAFETY: tests are single-threaded per process here.
+        // Hold the cross-module lock so concurrent env-mutating tests in
+        // sibling modules don't race the global process environment.
+        let _guard = super::TEST_ENV_LOCK
+            .lock()
+            .unwrap_or_else(|e| e.into_inner());
         std::env::set_var("FBUILD_TEENSY_DISABLE_BAUD_134_TRIGGER", "1");
         assert!(baud_134_trigger_disabled());
         std::env::set_var("FBUILD_TEENSY_DISABLE_BAUD_134_TRIGGER", "0");