Skip to content

fix(deploy): probe-rs spawns via run_command_blocking — no console flash, 120s timeout - #938

Merged
zackees merged 1 commit into
mainfrom
fix/probe-rs-no-window
Jul 2, 2026
Merged

fix(deploy): probe-rs spawns via run_command_blocking — no console flash, 120s timeout#938
zackees merged 1 commit into
mainfrom
fix/probe-rs-no-window

Conversation

@zackees

@zackeeszackees commented Jul 2, 2026

Copy link
Copy Markdown
Member

Problem

run_probe_rs_download / run_probe_rs_reset (crates/fbuild-deploy/src/probe_rs.rs) spawned probe-rs with raw std::process::Command::output(). The daemon is windowless on Windows, so every SWD flash allocated a fresh console for the console-subsystem probe-rs.exe — a visible console window flashed on the user's screen during each deploy. Observed live on an LPC845-BRK deploy today.

Two additional gaps from bypassing the blessed wrapper:

  • No timeout: a wedged probe (the exact CMSIS-DAP v1.0.7 failure family this path exists to mitigate) would hold the daemon's spawn_blocking slot forever.
  • No MSYS env stripping.

Fix

Route both wrappers through fbuild_core::subprocess::run_command_blocking — the house-standard spawn path (ban_raw_subprocess dylint) that already sets CREATE_NO_WINDOW (0x08000000) on Windows, strips MSYS env vars, and applies a timeout. New PROBE_RS_TIMEOUT = 120 s: a healthy LPC845-BRK flash completes in ~2 s; past 120 s the probe is wedged and the deploy fails with the captured stderr instead of hanging.

ToolOutput {stdout, stderr, exit_code} maps 1:1 onto ProbeRsRun, so callers are unchanged.

Why the dylint didn't catch this

ban_raw_subprocess covers exactly this pattern and probe_rs.rs is not allowlisted — but the Dylint CI workflow has been failing on main at the driver-build step (dylint::library_packages::build_library, toolchain-alias retry exhaustion) for at least the last 5 runs. Red gate = zero enforcement, and #935 merged through the gap. Filing the CI-infra issue separately; this PR just removes the violation.

Validation

  • soldr cargo check -p fbuild-deploy — clean.
  • Hardware: LPC845-BRK on COM10, Windows 10, LPC-Link2 CMSIS-DAP v1.0.7. fbuild deploy -e lpc845 via the FastLED autoresearch harness: flash completes (~2 s SWD via probe-rs), VCOM reopens immediately, freshly-flashed firmware answers JSON-RPC. Console-flash absence verified visually on a post-fix deploy.

Refs: #935, #936 (probe-rs SWD dispatch), #264 (ban_raw_subprocess).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability when running probe-rs actions by enforcing a timeout, helping prevent commands from hanging indefinitely.
    • Simplified command result handling so download and reset operations report exit status and output more consistently.

…TE_NO_WINDOW + timeout)
`run_probe_rs_download` / `run_probe_rs_reset` used raw
`std::process::Command::output()`. The daemon is windowless on
Windows, so each probe-rs invocation allocated a fresh console —
a visible console window flashed on the user's screen for the
duration of every SWD flash.
Route both through `fbuild_core::subprocess::run_command_blocking`,
which is the blessed wrapper (`ban_raw_subprocess` dylint) and
already sets `CREATE_NO_WINDOW` on Windows, strips MSYS env vars,
and applies a timeout. New `PROBE_RS_TIMEOUT` = 120 s: a healthy
LPC845-BRK flash completes in ~2 s; past 120 s the probe is wedged
and the deploy should fail with captured stderr instead of holding
the daemon's spawn_blocking slot forever.
Why the dylint didn't catch this at merge time: the Dylint CI
workflow has been failing on main at the driver-build step
(dylint::library_packages::build_library), so the gate was not
enforcing when #935 landed. Separate issue to follow for the CI
infra fix.
Refs: #935, #936 (probe-rs SWD dispatch), #264 (ban_raw_subprocess).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 86396255-8890-4cb4-955f-6d6b71d54f49

📥 Commits

Reviewing files that changed from the base of the PR and between 64f8950 and aa78800.

📒 Files selected for processing (1)
  • crates/fbuild-deploy/src/probe_rs.rs

📝 Walkthrough

Walkthrough

Refactors probe_rs.rs to replace direct std::process::Command usage with the run_command_blocking helper, adding a 120-second PROBE_RS_TIMEOUT constant. Both run_probe_rs_download and run_probe_rs_reset now build ProbeRsRun directly from the helper's exit_code, stdout, and stderr fields.

Changes

Probe-rs subprocess timeout migration

Layer / File(s)Summary
Timeout constant and helper import
crates/fbuild-deploy/src/probe_rs.rs
Adds Duration and run_command_blocking imports, and defines PROBE_RS_TIMEOUT (120s) as a hard ceiling for probe-rs subprocess execution.
Apply timeout to download and reset flows
crates/fbuild-deploy/src/probe_rs.rs
Updates run_probe_rs_download and run_probe_rs_reset to use run_command_blocking with PROBE_RS_TIMEOUT, convert argv to &str refs, and build ProbeRsRun from the helper's exit_code/stdout/stderr instead of manual Command output decoding.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
participant Caller
participant ProbeRsModule as run_probe_rs_download/run_probe_rs_reset
participant Subprocess as run_command_blocking
Caller->>ProbeRsModule: invoke probe-rs command
ProbeRsModule->>Subprocess: run_command_blocking(argv_refs, Some(PROBE_RS_TIMEOUT))
Subprocess-->>ProbeRsModule: exit_code, stdout, stderr
ProbeRsModule-->>Caller: ProbeRsRun
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title accurately summarizes the switch to run_command_blocking and the added 120-second timeout for probe-rs execution.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/probe-rs-no-window

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zackees
zackees merged commit 686e5d4 into mainJul 2, 2026
87 of 93 checks passed
zackees added a commit that referenced this pull request Jul 2, 2026
`soldr cargo fmt` on lpc.rs (from #935's .with_probe_rs_chip chain) and
probe_rs.rs (from #938's run_command_blocking calls). Mechanical only.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

1 participant

@zackees