Uh oh!
There was an error while loading. Please reload this page.
fix(esp32): surface daemon errors + provision platform.json helper toolchains - #402
Conversation
…olchains Addresses fbuild#401. Daemon streaming (fbuild-daemon, fbuild-cli): - StreamTerminationGuard wraps the spawned build task with a Drop impl that enqueues a fallback terminal `result` NDJSON event if the task panics or returns without marking completion. Without this, the body stream closes mid-frame and the CLI reports the opaque reqwest error `stream error: error decoding response body` with no clue what failed. - CLI now wraps the underlying stream error with an actionable message pointing at ~/.fbuild/daemon/daemon.log instead of bubbling the raw reqwest text. ESP32 platform.json package enumeration (fbuild-packages, fbuild-build): - Esp32Platform::enumerate_packages() returns (name, version_url) pairs for every entry in the `packages` section, sorted by name. - The pioarduino orchestrator now opportunistically provisions every toolchain-* helper listed in platform.json that isn't the MCU-primary toolchain. Resolution failures are warned, never fatal, because not every build references every helper (e.g. only sketches with ULP code on ESP32-S3 actually need toolchain-riscv32-esp). - Version-compatibility policy: we always trust platform.json's pinned metadata URL rather than reuse a newer cached toolchain. This matches PlatformIO behavior and avoids ad-hoc version-compatibility decisions. Tests: - esp32_platform: 5 new unit tests against a pioarduino 54.03.20 packages-section fixture, verifying both the MCU-primary Xtensa URL and the RISC-V helper URL for ESP32-S3 are resolvable, plus that enumerate_packages returns the full sorted list and errors cleanly when `packages` is missing. - build handler: 2 new tests for StreamTerminationGuard — Drop emits a fallback `result` event when not marked completed, and stays silent after mark_completed().
Warning Review limit reached
More reviews will be available in 42 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR provisions helper ESP toolchains discovered in ChangesESP Toolchain Resolution and Build Streaming
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/fbuild-packages/src/library/esp32_platform.rs (1)
119-152: 💤 Low valueOptional: factor out the shared platform.json read/parse.
enumerate_packagesduplicates the read-string +serde_json::from_str+get("packages")boilerplate already present inget_package_url(Lines 83-108). A small private helper returning the parsedpackagesobject (or the fullValue) would remove the duplication and keep error messages consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-packages/src/library/esp32_platform.rs` around lines 119 - 152, Both enumerate_packages and get_package_url duplicate the platform.json read/parse/lookup logic; create a small private helper (e.g., parse_platform_json or load_platform_packages) that reads platform.json, deserializes to serde_json::Value (or returns the packages object), and returns Result<serde_json::Map<String, Value>, FbuildError> so both enumerate_packages and get_package_url call it; update enumerate_packages to call the new helper to obtain the packages map and then map/sort entries, and update get_package_url to use the same helper to fetch a package's version/url, preserving existing error messages.crates/fbuild-build/src/esp32/orchestrator/packages.rs (2)
94-95: 💤 Low valueHoist
Cache::newout of the loop.
fbuild_packages::Cache::new(project_dir)is reconstructed on every iteration but only depends onproject_dir. Create it once before the loop.♻️ Proposed change
+ let cache = fbuild_packages::Cache::new(project_dir); for (name, metadata_url) in entries { if name == primary || !name.starts_with("toolchain-") { continue; } - let cache = fbuild_packages::Cache::new(project_dir); let cache_dir = cache.toolchains_dir().join(&name);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-build/src/esp32/orchestrator/packages.rs` around lines 94 - 95, Hoist the creation of the package cache so it is not reconstructed each loop: move the call to fbuild_packages::Cache::new(project_dir) out of the loop and store it in a local variable (e.g. let cache = fbuild_packages::Cache::new(project_dir)) before iterating; inside the loop continue to call cache.toolchains_dir().join(&name) (using the existing name variable) so only one Cache instance is created rather than recreating it on every iteration.
90-118: ⚖️ Poor tradeoffSynchronous helper-toolchain resolution runs on every ESP32 build.
provision_helper_toolchainscallsresolve_toolchain_url_syncfor each non-primarytoolchain-*entry on the build path, even though the comment notes most FastLED sketches never compile ULP code. On a cold cache this adds blocking metadata round-trips (and potential download stalls) to builds that will never use the helper. Consider gating eager provisioning behind a flag/board-capability check, or moving it off the critical path (background/lazy on first ULP reference), so the common case isn't penalized.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-build/src/esp32/orchestrator/packages.rs` around lines 90 - 118, The loop in provision_helper_toolchains synchronously calls resolve_toolchain_url_sync for every non-primary toolchain entry, blocking ESP32 builds; change this to avoid eager blocking by either (A) gating the loop behind a new config/flag or a board-capability check so you only provision when ULP/helper toolchains are actually needed, or (B) move the provisioning off the critical path by spawning a background task (or using an async resolver) to call resolve_toolchain_url_* for each entry and logging results, or (C) defer resolution to a lazy on-first-use path where the code that needs the helper toolchain triggers resolution on demand; update places referencing Cache::new and cache.toolchains_dir() accordingly to be usable from the chosen background/async or gated flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-cli/src/daemon_client.rs`:
- Around line 516-520: The error message currently hardcodes
"~/.fbuild/daemon/daemon.log"; replace that with the resolved daemon log path
from the fbuild-paths crate so users in dev/prod isolation get the correct
location. In the FbuildError::DaemonError construction (the block that formats
the message using `e`), call the fbuild-paths API to get the daemon log path
(e.g., fbuild_paths::daemon_log_path() or the appropriate function in
fbuild_paths::paths) and interpolate that returned path into the formatted
string instead of the hardcoded "~/.fbuild/daemon/daemon.log".
---
Nitpick comments:
In `@crates/fbuild-build/src/esp32/orchestrator/packages.rs`:
- Around line 94-95: Hoist the creation of the package cache so it is not
reconstructed each loop: move the call to
fbuild_packages::Cache::new(project_dir) out of the loop and store it in a local
variable (e.g. let cache = fbuild_packages::Cache::new(project_dir)) before
iterating; inside the loop continue to call cache.toolchains_dir().join(&name)
(using the existing name variable) so only one Cache instance is created rather
than recreating it on every iteration.
- Around line 90-118: The loop in provision_helper_toolchains synchronously
calls resolve_toolchain_url_sync for every non-primary toolchain entry, blocking
ESP32 builds; change this to avoid eager blocking by either (A) gating the loop
behind a new config/flag or a board-capability check so you only provision when
ULP/helper toolchains are actually needed, or (B) move the provisioning off the
critical path by spawning a background task (or using an async resolver) to call
resolve_toolchain_url_* for each entry and logging results, or (C) defer
resolution to a lazy on-first-use path where the code that needs the helper
toolchain triggers resolution on demand; update places referencing Cache::new
and cache.toolchains_dir() accordingly to be usable from the chosen
background/async or gated flow.
In `@crates/fbuild-packages/src/library/esp32_platform.rs`:
- Around line 119-152: Both enumerate_packages and get_package_url duplicate the
platform.json read/parse/lookup logic; create a small private helper (e.g.,
parse_platform_json or load_platform_packages) that reads platform.json,
deserializes to serde_json::Value (or returns the packages object), and returns
Result<serde_json::Map<String, Value>, FbuildError> so both enumerate_packages
and get_package_url call it; update enumerate_packages to call the new helper to
obtain the packages map and then map/sort entries, and update get_package_url to
use the same helper to fetch a package's version/url, preserving existing error
messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 007c9927-1799-4c6f-a5b8-14dddb46f48f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/fbuild-build/src/esp32/orchestrator/packages.rscrates/fbuild-cli/src/daemon_client.rscrates/fbuild-daemon/src/handlers/operations/build.rscrates/fbuild-packages/src/library/esp32_platform.rs
Uh oh!
There was an error while loading. Please reload this page.
| let event = serde_json::json!({ | ||
| "type": "result", | ||
| "success": false, | ||
| "request_id": self.request_id, | ||
| "message": "daemon build worker terminated unexpectedly (panic or early return); check ~/.fbuild/daemon/daemon.log", | ||
| "exit_code": 1, | ||
| "output_file": null, | ||
| "output_dir": null, | ||
| }); |
There was a problem hiding this comment.
Use env-aware daemon log path in fallback result message.
Line 46 hardcodes ~/.fbuild/daemon/daemon.log, but log location is env-scoped. In dev mode this points users to the wrong file and weakens the new actionable diagnostic.
💡 Suggested patch
- let event = serde_json::json!({+ let daemon_log = fbuild_paths::get_daemon_log_file();+ let event = serde_json::json!({
"type": "result",
"success": false,
"request_id": self.request_id,
- "message": "daemon build worker terminated unexpectedly (panic or early return); check ~/.fbuild/daemon/daemon.log",+ "message": format!(+ "daemon build worker terminated unexpectedly (panic or early return); check {}",+ daemon_log.display()+ ),
"exit_code": 1,
"output_file": null,
"output_dir": null,
});Based on learnings: fbuild-paths implements dev/prod path isolation as ~/.fbuild/{dev|prod}/.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let event = serde_json::json!({ | |
| "type":"result", | |
| "success": false, | |
| "request_id":self.request_id, | |
| "message":"daemon build worker terminated unexpectedly (panic or early return); check ~/.fbuild/daemon/daemon.log", | |
| "exit_code":1, | |
| "output_file": null, | |
| "output_dir": null, | |
| }); | |
| let daemon_log = fbuild_paths::get_daemon_log_file(); | |
| let event = serde_json::json!({ | |
| "type":"result", | |
| "success": false, | |
| "request_id":self.request_id, | |
| "message": format!( | |
| "daemon build worker terminated unexpectedly (panic or early return); check {}", | |
| daemon_log.display() | |
| ), | |
| "exit_code":1, | |
| "output_file": null, | |
| "output_dir": null, | |
| }); |
- daemon_client: resolve daemon-log path via fbuild_paths::get_daemon_log_file instead of hardcoding ~/.fbuild/daemon/daemon.log so dev-mode users see the correct path. - esp32_platform: factor shared platform.json read/parse/lookup into a private read_packages_section() helper used by both get_package_url and enumerate_packages. - orchestrator/packages: hoist Cache::new out of the helper-toolchain loop; skip resolve_toolchain_url_sync entirely when the cache directory already exists so steady-state cost on a warm cache is zero (addresses the "synchronous resolution runs on every ESP32 build" concern).
All three actionable concerns addressed in commit 7b48748: (1) shared platform.json parsing factored into read_packages_section helper; (2) Cache::new hoisted out of the helper-toolchain loop with cache-aware skip when the toolchain dir already exists (zero steady-state cost); (3) hardcoded ~/.fbuild/daemon/daemon.log replaced with fbuild_paths::get_daemon_log_file(). All 10 esp32_platform tests + 2 termination guard tests still pass.
Uh oh!
There was an error while loading. Please reload this page.
Closes#401.
Summary
Two complementary fixes for ESP toolchain handling raised in #401:
Daemon streaming surfaces a usable error (was:
stream error: error decoding response body)The streaming
/api/buildhandler runs the build in a spawned task that emits a terminal NDJSONresultevent when it finishes. If that task panicked or returned early, no terminal event was emitted, the response body stream closed mid-frame, and the CLI surfaced reqwest's underlyingstream error: error decoding response body— an opaque message that gave the user no actionable diagnostic.StreamTerminationGuardinfbuild-daemonwraps the spawn body. OnDropwithout an explicitmark_completed(), it enqueues a fallback terminalresultevent pointing the user at~/.fbuild/daemon/daemon.log. The happy path callsmark_completed()after the real result event so no duplicate is emitted.fbuild-cli'sbuild_streamingwraps the reqwest chunk error with an actionable hint instead of the raw message.ESP32 platform.json helper toolchains are now provisioned
Esp32Platform::get_toolchain_metadata_urlpreviously returned only the MCU-primary toolchain (toolchain-xtensa-esp-elfORtoolchain-riscv32-esp). pioarduinoplatform-espressif32@54.03.20lists both on the same platform.json — the RISC-V toolchain is needed on ESP32-S3 because the chip has a RISC-V ULP coprocessor.Esp32Platform::enumerate_packages()returns(name, version_url)for every entry inpackages, sorted by name.resolve_pioarduino_packagesnow opportunistically resolves every additionaltoolchain-*listed inplatform.jsonafter the primary one. Resolution failures log a warning but don't fail the build, because not every sketch references every helper (most FastLED sketches don't compile ULP code).platform.json's pinned metadata URL (option b from the issue), matching PlatformIO behavior and avoiding ad-hoc version-compatibility decisions.Tests added
esp32_platform(5 new unit tests, all green locally):test_get_toolchain_metadata_url_xtensa_pioarduino_54_03_20— primary Xtensa URL resolves.test_get_riscv_helper_toolchain_for_esp32s3_pioarduino_54_03_20— RISC-V helper URL is discoverable on an ESP32-S3 build.test_enumerate_packages_returns_all_entries— sorted full list including helpers + esptoolpy.test_enumerate_packages_errors_when_section_missing— clean error on malformed manifest.fbuild-daemon::handlers::operations::build(2 new unit tests):termination_guard_emits_fallback_on_drop_when_not_completed— Drop enqueues a fallbackresultevent with an actionable message.termination_guard_silent_on_drop_when_completed—mark_completed()disarms the guard so no duplicate is emitted.All four touched crates lint clean (
soldr cargo clippy --workspace --all-targets -- -D warnings).Acceptance criteria mapping (from #401)
enumerate_packages+ opportunistic provisioning.tests/platform/esp32s3/) continues to use the stockplatform = espressif32channel as before.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests