Skip to content

fix(esp32): fingerprint the esptool resolution in the BIN reuse check - #1240

Merged
zackees merged 1 commit into
mainfrom
fix/1238-bincache-fingerprint
Aug 2, 2026
Merged

fix(esp32): fingerprint the esptool resolution in the BIN reuse check#1240
zackees merged 1 commit into
mainfrom
fix/1238-bincache-fingerprint

Conversation

@zackees

@zackeeszackees commented Aug 2, 2026

Copy link
Copy Markdown
Member

Closes#1238

What

Since #1236, the ESP32 elf2image esptool spawn resolves against the per-request caller PATH — but can_reuse_bin / BinArtifactCache recorded nothing about which esptool produced the cached firmware.bin. Two requests with different caller PATHs can resolve bare-name esptool to different binaries (venv vs system, different versions), and the second silently reused the first one's bin. CodeRabbit originally flagged and confirmed this on the superseded PR #1231.

How

  • BinArtifactCache gains #[serde(default)] esptool_fingerprint: String.
  • Empty for a provisioned absolute-path esptool or no caller PATH (resolution cannot drift → pre-existing cache records stay valid, caching behaves exactly as before); a 16-hex-char SHA-256 prefix of the caller PATH for bare-name spawns.
  • Recorded in current_bin_cache, compared by the existing struct equality in can_reuse_bin.
  • No BUILD_FINGERPRINT_VERSION bump: it's shared by unrelated caches, and the serde default already yields correct match semantics (old records deserialize as "", mismatching any caller-PATH fingerprint — invalidation only in the correct direction).
  • Also adds the end-to-end run_command test CodeRabbit requested on fix(daemon): forward the caller CLI's PATH per request for bare-name tool spawns #1231: a uniquely-named probe executable staged in a TempDir proves bare-name resolution comes from the overlay PATH (and fails without it), while absolute-path spawns are unaffected.

RED → GREEN

RED on current main (exit=101):

test esp32::esp32_linker::tests::bare_name_esptool_bin_reuse_is_keyed_by_caller_path ... FAILED
panicked: a different caller PATH must not reuse a bin produced by another PATH's esptool

GREEN after the fix: that test plus absolute_esptool_bin_reuse_ignores_caller_path and run_command_resolves_bare_name_from_overlay_path all pass.

Tests

soldr cargo test -p fbuild-build-engine -p fbuild-core -p fbuild-build-esp — 751 tests, 0 failures. soldr cargo clippy --workspace --all-targets -- -D warnings clean; fmt applied.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved firmware build caching when esptool is resolved through different PATH configurations.
    • Prevented stale cache reuse when the selected esptool executable changes.
    • Preserved consistent caching for explicitly specified esptool paths.
    • Improved command execution when resolving tools through an overlaid PATH.

Closes#1238
BinArtifactCache gains a serde-defaulted esptool_fingerprint: empty for
a provisioned absolute-path esptool (resolution cannot drift, so
pre-existing cache records stay valid and caching behaves exactly as
before), a short SHA-256 of the caller's PATH for bare-name spawns.
Since #1236 the esptool spawn resolves against the per-request caller
PATH, so two requests with different caller PATHs can resolve different
esptool binaries — they must never share a cached firmware.bin.
The shared BUILD_FINGERPRINT_VERSION constant is left untouched:
bumping it would invalidate every unrelated cache, and the serde
default already yields the correct match semantics.
Also adds the end-to-end subprocess test proving run_command resolves a
bare executable from an overlay PATH (staged uniquely named probe in a
TempDir), that absolute-path spawns work without an overlay, and that
the bare name fails without the overlay.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Aug 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 Plus

Run ID: d4dd12e9-1e17-4697-9d47-8e894adc74ec

📥 Commits

Reviewing files that changed from the base of the PR and between e85fb71 and b9e56cc.

📒 Files selected for processing (3)
  • crates/fbuild-build-engine/src/build_fingerprint/mod.rs
  • crates/fbuild-build-esp/src/esp32/esp32_linker.rs
  • crates/fbuild-core/src/subprocess.rs

📝 Walkthrough

Walkthrough

The change adds backward-compatible esptool fingerprints to BIN cache records. Bare-name resolution fingerprints the caller PATH, while absolute paths remain PATH-independent. Tests cover cache reuse and subprocess resolution behavior.

Changes

Esptool cache fingerprinting

Layer / File(s)Summary
Cache contract and fingerprint wiring
crates/fbuild-build-engine/src/build_fingerprint/mod.rs, crates/fbuild-build-esp/src/esp32/esp32_linker.rs
BinArtifactCache stores a defaultable esptool_fingerprint. ESP32 linker cache state records an empty fingerprint for absolute paths and a SHA-256-derived fingerprint for bare-name resolution.
Resolution and regression validation
crates/fbuild-build-esp/src/esp32/esp32_linker.rs, crates/fbuild-core/src/subprocess.rs
Tests verify PATH-sensitive cache reuse, PATH-independent absolute-path reuse, and subprocess resolution through an overlaid PATH.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

Possibly related PRs

  • FastLED/fbuild#1231: This PR extends its Esp32Linker and BinArtifactCache changes with PATH-sensitive esptool fingerprinting.
  • FastLED/fbuild#988: This PR extends its esptool path-resolution logic with PATH fingerprinting.
  • FastLED/fbuild#1016: This PR extends the build fingerprint and cache infrastructure introduced there.
🚥 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 clearly and concisely describes the main change: fingerprinting esptool resolution for ESP32 BIN reuse checks.
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/1238-bincache-fingerprint

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 83a327e into mainAug 2, 2026
93 checks passed
@zackees
zackees deleted the fix/1238-bincache-fingerprint branch August 2, 2026 03:49
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.

fix(esp32): BIN artifact cache ignores which esptool produced the bin — caller-PATH requests can reuse a stale firmware.bin (gap in #1236)

1 participant

@zackees