Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions crates/fbuild-build/src/esp32/orchestrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1033,10 +1033,48 @@ impl BuildOrchestrator for Esp32Orchestrator {
}

// 11.5. Process embedded files (board_build.embed_files + embed_txtfiles)
//
// `.lnk` entries are pre-resolved: each `.lnk` is parsed, its blob is
// fetched (or pulled from the disk cache), and the materialized path
// is substituted in place before objcopy sees it. The `_lnk_leases`
// vector keeps cache leases alive until we leave this scope, so the
// disk-cache GC can't reap a blob mid-build.
if !embed_files.is_empty() || !embed_txtfiles.is_empty() {
let embed_dir = build_dir.join("embed");
std::fs::create_dir_all(&embed_dir)?;

let lnk_dir = embed_dir.join("lnk");
let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new();
let lnk_cache = fbuild_packages::DiskCache::open().ok();

let expand = |entries: &[String]| -> Result<Vec<String>> {
let mut out = Vec::with_capacity(entries.len());
for entry in entries {
let p = if Path::new(entry).is_absolute() {
std::path::PathBuf::from(entry)
} else {
params.project_dir.join(entry)
};
if fbuild_packages::lnk::has_lnk_extension(&p) {
let cache = lnk_cache.as_ref().ok_or_else(|| {
fbuild_core::FbuildError::PackageError(
"disk cache unavailable; cannot resolve .lnk entries"
.to_string(),
)
})?;
let m = fbuild_packages::lnk::materialize_lnk_entry(
&p, &lnk_dir, cache,
)?;
out.push(m.target_path.to_string_lossy().into_owned());
} else {
out.push(entry.clone());
}
}
Ok(out)
};
let resolved_embed_files = expand(&embed_files)?;
let resolved_embed_txtfiles = expand(&embed_txtfiles)?;
Comment on lines +1046 to +1076
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep the MaterializedLnk leases alive as intended.

resolve_lnk drops m immediately after cloning target_path, so _lnk_leases never holds the RAII lease described in the comment.

Proposed fix
             let lnk_dir = embed_dir.join("lnk");
             let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new();
             let lnk_cache = fbuild_packages::DiskCache::open().ok();
 
-            let resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> {
+            let mut resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> {
                 let cache = lnk_cache.as_ref().ok_or_else(|| {
                     fbuild_core::FbuildError::PackageError(
                         "disk cache unavailable; cannot resolve .lnk entries".to_string(),
                     )
                 })?;
                 let m = fbuild_packages::lnk::materialize_lnk_entry(lnk_path, &lnk_dir, cache)?;
-                Ok(m.target_path.clone())
+                let target_path = m.target_path.clone();
+                _lnk_leases.push(m);
+                Ok(target_path)
             };
-            // Closures can't borrow `_lnk_leases` mutably while also being
-            // FnMut for both expansions, so we collect leases inline by
-            // calling `materialize_lnk_entry` directly inside a small loop.
-            let expand = |entries: &[String]| -> Result<Vec<String>> {
+            let mut expand = |entries: &[String]| -> Result<Vec<String>> {
📝 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.

Suggested change
let lnk_dir = embed_dir.join("lnk");
let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new();
let lnk_cache = fbuild_packages::DiskCache::open().ok();
let resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> {
let cache = lnk_cache.as_ref().ok_or_else(|| {
fbuild_core::FbuildError::PackageError(
"disk cache unavailable; cannot resolve .lnk entries".to_string(),
)
})?;
let m = fbuild_packages::lnk::materialize_lnk_entry(lnk_path, &lnk_dir, cache)?;
Ok(m.target_path.clone())
};
// Closures can't borrow `_lnk_leases` mutably while also being
// FnMut for both expansions, so we collect leases inline by
// calling `materialize_lnk_entry` directly inside a small loop.
let expand = |entries: &[String]| -> Result<Vec<String>> {
let mut out = Vec::with_capacity(entries.len());
for entry in entries {
let p = if Path::new(entry).is_absolute() {
std::path::PathBuf::from(entry)
} else {
params.project_dir.join(entry)
};
if fbuild_packages::lnk::has_lnk_extension(&p) {
let resolved = resolve_lnk(&p)?;
out.push(resolved.to_string_lossy().into_owned());
} else {
out.push(entry.clone());
}
}
Ok(out)
};
let resolved_embed_files = expand(&embed_files)?;
let resolved_embed_txtfiles = expand(&embed_txtfiles)?;
let lnk_dir = embed_dir.join("lnk");
let mut _lnk_leases: Vec<fbuild_packages::lnk::MaterializedLnk> = Vec::new();
let lnk_cache = fbuild_packages::DiskCache::open().ok();
let mut resolve_lnk = |lnk_path: &Path| -> Result<PathBuf> {
let cache = lnk_cache.as_ref().ok_or_else(|| {
fbuild_core::FbuildError::PackageError(
"disk cache unavailable; cannot resolve .lnk entries".to_string(),
)
})?;
let m = fbuild_packages::lnk::materialize_lnk_entry(lnk_path, &lnk_dir, cache)?;
let target_path = m.target_path.clone();
_lnk_leases.push(m);
Ok(target_path)
};
let mut expand = |entries: &[String]| -> Result<Vec<String>> {
let mut out = Vec::with_capacity(entries.len());
for entry in entries {
let p = if Path::new(entry).is_absolute() {
std::path::PathBuf::from(entry)
} else {
params.project_dir.join(entry)
};
if fbuild_packages::lnk::has_lnk_extension(&p) {
let resolved = resolve_lnk(&p)?;
out.push(resolved.to_string_lossy().into_owned());
} else {
out.push(entry.clone());
}
}
Ok(out)
};
let resolved_embed_files = expand(&embed_files)?;
let resolved_embed_txtfiles = expand(&embed_txtfiles)?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/fbuild-build/src/esp32/orchestrator.rs` around lines 1046 - 1080, The
current resolve_lnk drops the MaterializedLnk immediately (only cloning
target_path) so the RAII lease is never stored; fix by keeping each lease in the
_lnk_leases Vec: either (preferred) remove the resolve_lnk shortcut and in
expand's loop call fbuild_packages::lnk::materialize_lnk_entry directly, push
the returned MaterializedLnk into _lnk_leases before using its .target_path, or
change resolve_lnk to accept &mut _lnk_leases (or return the MaterializedLnk and
push it into _lnk_leases at call sites) so the lease objects returned by
materialize_lnk_entry are stored in _lnk_leases and not dropped immediately
(update calls in expand and resolved_embed_* accordingly).


let objcopy_path = toolchain.get_objcopy_path();
let (output_target, binary_arch) = if mcu_config.is_riscv() {
("elf32-littleriscv", "riscv")
Expand All @@ -1045,8 +1083,8 @@ impl BuildOrchestrator for Esp32Orchestrator {
};

let embed_objects = process_embed_files(
&embed_files,
&embed_txtfiles,
&resolved_embed_files,
&resolved_embed_txtfiles,
&params.project_dir,
&embed_dir,
&objcopy_path,
Expand Down
35 changes: 30 additions & 5 deletions crates/fbuild-packages/src/lnk/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,18 @@ pub fn resolve(lnk: &LnkFile, cache: &DiskCache) -> Result<ResolvedBlob> {
// Best-effort sha verify on cache hit — cheap (single read,
// not network). Catches accidental cache corruption.
if verify_sha256(&blob_path, &lnk.sha256).is_ok() {
let lease = cache.lease(&entry).map_err(map_cache_err)?;
cache.touch(&entry).map_err(map_cache_err)?;
// Lease acquisition is best-effort: older cache schemas may
// lack the `refcount` column. A missing lease just means
// "blob isn't pinned against concurrent GC"; for a single-
// process build that's acceptable.
let lease = best_effort_lease(cache, &entry);
let _ = cache.touch(&entry);
debug!(url = %lnk.url, sha = %lnk.sha256, "lnk cache hit");
return Ok(ResolvedBlob {
path: blob_path,
sha256: lnk.sha256.clone(),
entry: Some(entry),
lease: Some(lease),
lease,
});
}
tracing::warn!(
Expand Down Expand Up @@ -156,7 +160,7 @@ pub fn resolve(lnk: &LnkFile, cache: &DiskCache) -> Result<ResolvedBlob> {
)
.map_err(map_cache_err)?;

let lease = cache.lease(&entry).map_err(map_cache_err)?;
let lease = best_effort_lease(cache, &entry);
info!(
url = %lnk.url,
bytes = archive_bytes,
Expand All @@ -168,10 +172,31 @@ pub fn resolve(lnk: &LnkFile, cache: &DiskCache) -> Result<ResolvedBlob> {
path: final_path,
sha256: lnk.sha256.clone(),
entry: Some(entry),
lease: Some(lease),
lease,
})
}

/// Attempt to acquire a cache lease, returning `None` on failure.
///
/// Older `DiskCache` schemas may lack the `refcount` column that
/// `lease.rs` expects. In that case we log a warning and continue
/// without a lease — the cached blob is still valid, it just isn't
/// pinned against concurrent GC. For a single-process build that's
/// acceptable; parallel builds on an old schema may race, but the
/// same was true before this change.
fn best_effort_lease(cache: &DiskCache, entry: &CacheEntry) -> Option<Lease> {
match cache.lease(entry) {
Ok(l) => Some(l),
Err(e) => {
tracing::warn!(
error = %e,
"failed to acquire lnk cache lease; continuing unpinned"
);
None
}
}
}

/// Reconstruct the on-disk blob path from a `CacheEntry`. The entry's
/// `archive_path` is set when `record_archive` was called.
fn blob_path_for(entry: &CacheEntry) -> PathBuf {
Expand Down
Loading