From b66af389cdc8935c7a064639d173987883a1a387 Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 19:19:58 -0700 Subject: [PATCH 1/4] feat(daemon): adopt mimalloc-pprof and prove heap, on-CPU, and off-CPU profiling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes FastLED/fbuild#1361. Motivated by #1360, where `fbuild-daemon` grew to ~3.9 GB resident while idle and then failed its own health check — and the only tool that could have said *what* was holding the memory was not compiled into the binary. ## The allocator `fbuild-daemon` swaps `mimalloc` for `mimalloc-pprof`: the same mimalloc it already used, with a sampled heap profiler attached. Not feature-gated, on purpose. A profiler compiled out of the shipped binary is never present on the machine where a slow leak reproduces — #1360 was found on someone's real Windows box, twice in one session, and is still not minimally reproducible. The sampler is dormant until started, so the steady-state cost is the allocator swap alone. Note the issue's premise needed correcting: it proposed routing through zccache's `heap_profile` feature, but the pinned zccache (1.13.1, `8cf6dd0`) has no such feature and no `mimalloc-pprof` — those exist only on zccache main. `mimalloc-pprof` is a crates.io crate and zccache's module is a plain re-export, so depending on it directly gets the identical allocator with no zccache repin, which would have dragged `running-process` along with it. ## Two ways in, because leaks are found at two different times - `FBUILD_HEAP_PROFILE=1` (or a byte rate) starts sampling before any heavy init. Startup and static-init allocations are only visible this way. - `POST /api/daemon/heap-dump` works on a daemon that is *already* wedged. This is the half #1360 actually needed: restarting the daemon destroys the leak, so a restart-only profiler cannot answer the question it exists for. When profiling was not already on, the response says so rather than letting a thin profile read as "nothing is leaking". Dumps are pprof `profile.proto`, written under the dev/prod-isolated root via `fbuild_paths` rather than the daemon's CWD. ## Tests — all three modes, not just the wiring - `fbuild-daemon/tests/profiling.rs` — heap. Its `#[global_allocator]` is not incidental: an integration test is its own final executable, so it is the downstream linkage contract the daemon relies on. - `fbuild-core/tests/cpu_profiling.rs` — on-CPU (sampled stacks over a busy thread, then symbolized through `ModuleResolver`) and off-CPU (the async pipeline, asserting it ranks a task that waits 9s above one that burns 500x more CPU — that inversion is the whole point of an off-CPU profile). Two findings worth recording, both of which cost a debugging cycle: - The CPU tests live in `fbuild-core`, not next to the heap test, for a linker reason: `running-process-probe` pulls crash-handler 0.7 while the pinned zccache pulls 0.6.3, and both export the same unmangled C symbols (`ehsetjmp`, `handle_invalid_parameter`, ...). Any binary linking both fails with duplicate symbols. `fbuild-core` is the deepest crate that does not depend on zccache. - The heap test retains 2048 4 KiB blocks rather than one 4 MiB `Vec`. A single large allocation can take mimalloc's large-object path, which does not always pass the sampling hook — that version passed or failed depending on arena state, identically for the same binary run twice. Many small live objects is also the shape a real leak has. `running-process-probe-daemon` needs rusqlite 0.32 while the workspace pinned 0.31, and `links = "sqlite3"` permits only one. Bumped the workspace pin; fbuild-packages (249), fbuild-toolchain (144), fbuild-library, and fbuild-packages-fetch all pass on it. Verified: workspace clippy `-D warnings` clean, fmt clean, both profiling suites stable across repeated runs. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 344 ++++++++++++++++++-- Cargo.toml | 17 +- crates/fbuild-core/Cargo.toml | 12 + crates/fbuild-core/tests/cpu_profiling.rs | 189 +++++++++++ crates/fbuild-daemon/Cargo.toml | 2 +- crates/fbuild-daemon/src/handlers/health.rs | 55 +++- crates/fbuild-daemon/src/heap_profile.rs | 190 +++++++++++ crates/fbuild-daemon/src/lib.rs | 2 + crates/fbuild-daemon/src/main.rs | 22 +- crates/fbuild-daemon/src/models.rs | 14 + crates/fbuild-daemon/tests/profiling.rs | 106 ++++++ 11 files changed, 918 insertions(+), 35 deletions(-) create mode 100644 crates/fbuild-core/tests/cpu_profiling.rs create mode 100644 crates/fbuild-daemon/src/heap_profile.rs create mode 100644 crates/fbuild-daemon/tests/profiling.rs diff --git a/Cargo.lock b/Cargo.lock index 340058a60..853f625f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -151,7 +151,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "edca88bc138befd0323b20752846e6587272d3b03b0343c8ea28a6f819e6e71f" dependencies = [ "async-trait", - "axum-core", + "axum-core 0.4.5", "base64", "bytes", "futures-util", @@ -161,7 +161,7 @@ dependencies = [ "hyper", "hyper-util", "itoa", - "matchit", + "matchit 0.7.3", "memchr", "mime", "percent-encoding", @@ -181,6 +181,39 @@ dependencies = [ "tracing", ] +[[package]] +name = "axum" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "31b698c5f9a010f6573133b09e0de5408834d0c82f8d7475a89fc1867a71cd90" +dependencies = [ + "axum-core 0.5.6", + "bytes", + "form_urlencoded", + "futures-util", + "http", + "http-body", + "http-body-util", + "hyper", + "hyper-util", + "itoa", + "matchit 0.8.4", + "memchr", + "mime", + "percent-encoding", + "pin-project-lite", + "serde_core", + "serde_json", + "serde_path_to_error", + "serde_urlencoded", + "sync_wrapper", + "tokio", + "tower", + "tower-layer", + "tower-service", + "tracing", +] + [[package]] name = "axum-core" version = "0.4.5" @@ -202,6 +235,25 @@ dependencies = [ "tracing", ] +[[package]] +name = "axum-core" +version = "0.5.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08c78f31d7b1291f7ee735c1c6780ccde7785daae9a9206026862dab7d8792d1" +dependencies = [ + "bytes", + "futures-core", + "http", + "http-body", + "http-body-util", + "mime", + "pin-project-lite", + "sync_wrapper", + "tower-layer", + "tower-service", + "tracing", +] + [[package]] name = "base64" version = "0.22.1" @@ -497,6 +549,20 @@ version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d07550c9036bf2ae0c684c4297d503f838287c83c53686d05370d0e139ae570" +[[package]] +name = "console-api" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e8599749b6667e2f0c910c1d0dff6901163ff698a52d5a39720f61b5be4b20d3" +dependencies = [ + "futures-core", + "prost", + "prost-types", + "tonic", + "tonic-prost", + "tracing-core", +] + [[package]] name = "const-oid" version = "0.10.2" @@ -567,6 +633,19 @@ dependencies = [ "parking_lot", ] +[[package]] +name = "crash-handler" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0df5c9639f4942eb7702b964b3f9adf03a55724a57558cc177407388a8b936e2" +dependencies = [ + "cfg-if", + "crash-context", + "libc", + "mach2", + "parking_lot", +] + [[package]] name = "crc32fast" version = "1.5.0" @@ -913,7 +992,7 @@ dependencies = [ "bytemuck", "esp-idf-part", "flate2", - "gimli", + "gimli 0.32.3", "libc", "log", "md-5 0.11.0", @@ -1193,6 +1272,7 @@ dependencies = [ "prost", "reqwest", "running-process", + "running-process-probe-daemon", "same-file", "serde", "serde_json", @@ -1214,7 +1294,7 @@ name = "fbuild-daemon" version = "2.5.20" dependencies = [ "async-trait", - "axum", + "axum 0.7.9", "base64", "bytes", "clap", @@ -1228,7 +1308,7 @@ dependencies = [ "fbuild-serial", "fbuild-test-support", "futures", - "mimalloc", + "mimalloc-pprof", "prost", "regex", "reqwest", @@ -1290,7 +1370,7 @@ name = "fbuild-library" version = "2.5.20" dependencies = [ "async-trait", - "axum", + "axum 0.7.9", "bzip2", "fbuild-config", "fbuild-core", @@ -1341,7 +1421,7 @@ name = "fbuild-packages" version = "2.5.20" dependencies = [ "async-trait", - "axum", + "axum 0.7.9", "bzip2", "fbuild-config", "fbuild-core", @@ -1373,7 +1453,7 @@ name = "fbuild-packages-fetch" version = "2.5.20" dependencies = [ "async-trait", - "axum", + "axum 0.7.9", "bzip2", "fbuild-config", "fbuild-core", @@ -1479,7 +1559,7 @@ name = "fbuild-toolchain" version = "2.5.20" dependencies = [ "async-trait", - "axum", + "axum 0.7.9", "bzip2", "fbuild-config", "fbuild-core", @@ -1567,6 +1647,20 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "framehop" +version = "0.13.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09a5a3f0acb82df800ca3aa50c0d60d286c5d13d4cfc3114b3a9663f13b032fe" +dependencies = [ + "arrayvec", + "cfg-if", + "fallible-iterator", + "gimli 0.31.1", + "macho-unwind-info", + "pe-unwind-info", +] + [[package]] name = "fs2" version = "0.4.3" @@ -1740,6 +1834,16 @@ dependencies = [ "wasip3", ] +[[package]] +name = "gimli" +version = "0.31.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" +dependencies = [ + "fallible-iterator", + "stable_deref_trait", +] + [[package]] name = "gimli" version = "0.32.3" @@ -1770,6 +1874,25 @@ dependencies = [ "regex-syntax", ] +[[package]] +name = "h2" +version = "0.4.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "839c0e8a181239723652be9062bb56ca5bf5f64011f73b623f6f4fc59086a228" +dependencies = [ + "atomic-waker", + "bytes", + "fnv", + "futures-core", + "futures-sink", + "http", + "indexmap", + "slab", + "tokio", + "tokio-util", + "tracing", +] + [[package]] name = "half" version = "2.7.1" @@ -1896,6 +2019,7 @@ dependencies = [ "bytes", "futures-channel", "futures-core", + "h2", "http", "http-body", "httparse", @@ -1924,6 +2048,19 @@ dependencies = [ "webpki-roots", ] +[[package]] +name = "hyper-timeout" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b90d566bffbce6a75bd8b09a05aa8c2cb1fabb6cb348f8840c9e4c90a0d83b0" +dependencies = [ + "hyper", + "hyper-util", + "pin-project-lite", + "tokio", + "tower-service", +] + [[package]] name = "hyper-util" version = "0.1.20" @@ -2273,16 +2410,6 @@ dependencies = [ "windows-link", ] -[[package]] -name = "libmimalloc-sys" -version = "0.1.44" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "667f4fec20f29dfc6bc7357c582d91796c169ad7e2fce709468aefeb2c099870" -dependencies = [ - "cc", - "libc", -] - [[package]] name = "libredox" version = "0.1.15" @@ -2297,9 +2424,9 @@ dependencies = [ [[package]] name = "libsqlite3-sys" -version = "0.28.0" +version = "0.30.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0c10584274047cb335c23d3e61bcef8e323adae7c5c8c760540f73610177fc3f" +checksum = "2e99fb7a497b1e3339bc746195567ed8d3e24945ecd636e3619d20b9de9e9149" dependencies = [ "cc", "pkg-config", @@ -2393,6 +2520,17 @@ dependencies = [ "libc", ] +[[package]] +name = "macho-unwind-info" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bb4bdc8b0ce69932332cf76d24af69c3a155242af95c226b2ab6c2e371ed1149" +dependencies = [ + "thiserror 2.0.18", + "zerocopy", + "zerocopy-derive", +] + [[package]] name = "matchers" version = "0.2.0" @@ -2408,6 +2546,12 @@ version = "0.7.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0e7465ac9959cc2b1404e8e2367b43684a6d13790fe23056cc8c6c5a6b7bcb94" +[[package]] +name = "matchit" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47e1ffaa40ddd1f3ed91f717a33c8c0ee23fff369e3aa8772b9605cc1d22f4c3" + [[package]] name = "md-5" version = "0.10.6" @@ -2466,12 +2610,12 @@ dependencies = [ ] [[package]] -name = "mimalloc" -version = "0.1.48" +name = "mimalloc-pprof" +version = "0.9.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e1ee66a4b64c74f4ef288bcbb9192ad9c3feaad75193129ac8509af543894fd8" +checksum = "05d30b36d87ca14bd04fec54438531e7ef88ee0f28b34d75a37ddc98ac3fa93d" dependencies = [ - "libmimalloc-sys", + "cc", ] [[package]] @@ -2700,6 +2844,19 @@ dependencies = [ "num-traits", ] +[[package]] +name = "pe-unwind-info" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "500fa4cdeacd98997c5865e3d0d1cb8fe7e9d7d75ecc775e07989a433a9a9a59" +dependencies = [ + "arrayvec", + "bitflags 2.11.0", + "thiserror 2.0.18", + "zerocopy", + "zerocopy-derive", +] + [[package]] name = "percent-encoding" version = "2.3.2" @@ -2717,6 +2874,26 @@ dependencies = [ "indexmap", ] +[[package]] +name = "pin-project" +version = "1.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2466b2336ed02bcdca6b294417127b90ec92038d1d5c4fbeac971a922e0e0924" +dependencies = [ + "pin-project-internal", +] + +[[package]] +name = "pin-project-internal" +version = "1.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c96395f0a926bc13b1c17622aaddda1ecb55d49c8f1bf9777e4d877800a43f8b" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "pin-project-lite" version = "0.2.17" @@ -3440,11 +3617,64 @@ dependencies = [ "x11rb", ] +[[package]] +name = "running-process-probe" +version = "4.8.1" +source = "git+https://github.com/zackees/running-process.git?rev=359b4e3d92660e54eb9b16b6e83de8ce26932ff8#359b4e3d92660e54eb9b16b6e83de8ce26932ff8" +dependencies = [ + "crash-handler 0.7.0", + "framehop", + "getrandom 0.4.2", + "libc", + "mach2", + "object 0.36.7", + "prost", + "prost-build", + "protox", + "thiserror 2.0.18", + "winapi", + "windows-sys 0.59.0", +] + +[[package]] +name = "running-process-probe-daemon" +version = "4.8.1" +source = "git+https://github.com/zackees/running-process.git?rev=359b4e3d92660e54eb9b16b6e83de8ce26932ff8#359b4e3d92660e54eb9b16b6e83de8ce26932ff8" +dependencies = [ + "axum 0.8.9", + "blake3", + "clap", + "console-api", + "flate2", + "getrandom 0.4.2", + "globset", + "interprocess", + "libc", + "prost", + "prost-build", + "prost-types", + "protox", + "regex", + "running-process", + "running-process-probe", + "rusqlite", + "serde", + "serde_json", + "subtle", + "sysinfo", + "thiserror 2.0.18", + "tokio", + "tokio-stream", + "tokio-util", + "tonic", + "tower-http 0.6.8", +] + [[package]] name = "rusqlite" -version = "0.31.0" +version = "0.32.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b838eba278d213a8beaf485bd313fd580ca4505a00d5871caeb1457c55322cae" +checksum = "7753b721174eb8ff87a9a0e799e2d7bc3749323e773db92e0984debb00019d6e" dependencies = [ "bitflags 2.11.0", "fallible-iterator", @@ -4041,6 +4271,17 @@ dependencies = [ "tokio", ] +[[package]] +name = "tokio-stream" +version = "0.1.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a3d06f0b082ba57c26b79407372e57cf2a1e28124f78e9479fe80322cf53420b" +dependencies = [ + "futures-core", + "pin-project-lite", + "tokio", +] + [[package]] name = "tokio-tungstenite" version = "0.24.0" @@ -4158,6 +4399,45 @@ version = "1.1.1+spec-1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "756daf9b1013ebe47a8776667b466417e2d4c5679d441c26230efd9ef78692db" +[[package]] +name = "tonic" +version = "0.14.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac2a5518c70fa84342385732db33fb3f44bc4cc748936eb5833d2df34d6445ef" +dependencies = [ + "async-trait", + "base64", + "bytes", + "h2", + "http", + "http-body", + "http-body-util", + "hyper", + "hyper-timeout", + "hyper-util", + "percent-encoding", + "pin-project", + "socket2", + "sync_wrapper", + "tokio", + "tokio-stream", + "tower", + "tower-layer", + "tower-service", + "tracing", +] + +[[package]] +name = "tonic-prost" +version = "0.14.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50849f68853be452acf590cde0b146665b8d507b3b8af17261df47e02c209ea0" +dependencies = [ + "bytes", + "prost", + "tonic", +] + [[package]] name = "tower" version = "0.5.3" @@ -4166,9 +4446,12 @@ checksum = "ebe5ef63511595f1344e2d5cfa636d973292adc0eec1f0ad45fae9f0851ab1d4" dependencies = [ "futures-core", "futures-util", + "indexmap", "pin-project-lite", + "slab", "sync_wrapper", "tokio", + "tokio-util", "tower-layer", "tower-service", "tracing", @@ -4202,6 +4485,7 @@ dependencies = [ "futures-util", "http", "http-body", + "http-body-util", "iri-string", "pin-project-lite", "tower", @@ -5083,7 +5367,7 @@ dependencies = [ "blake3", "bytes", "clang", - "crash-handler", + "crash-handler 0.6.3", "dashmap", "filetime", "flate2", @@ -5185,7 +5469,7 @@ name = "zccache-core" version = "1.13.1" source = "git+https://github.com/zackees/zccache?rev=8cf6dd0ef5d6f2ff2980039290b9e498fb5a6b0e#8cf6dd0ef5d6f2ff2980039290b9e498fb5a6b0e" dependencies = [ - "crash-handler", + "crash-handler 0.6.3", "libc", "serde", "serde_json", @@ -5204,7 +5488,7 @@ dependencies = [ "blake3", "bytes", "clang", - "crash-handler", + "crash-handler 0.6.3", "dashmap", "filetime", "fs2", diff --git a/Cargo.toml b/Cargo.toml index 72a99546c..c28ccff5a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -127,9 +127,22 @@ async-trait = "0.1" dashmap = "6" blake3 = "1" criterion = { version = "0.5", default-features = false, features = ["html_reports"] } -mimalloc = "0.1" +# Global allocator for `fbuild-daemon`. `mimalloc-pprof` vendors the same +# mimalloc (v3 line) plus a sampled heap profiler that stays dormant until +# started, so this is the plain allocator it replaced with a reporting half +# attached — see FastLED/fbuild#1361 and the ~3.9 GB idle daemon in #1360. +# Not feature-gated on purpose: a profiler compiled out of the shipped +# binary is never present on the machine where a slow leak reproduces. +mimalloc-pprof = "0.9" + +# On-CPU sampling, stack symbolization, and the off-CPU/async profile +# pipeline. Dev-only: this is how the profiling contract is tested, not +# something the shipped daemon links. The git rev MUST equal the +# `running-process` rev pinned above, for the same `rp_*_public` +# symbol-duplication reason documented there. +running-process-probe-daemon = { version = "4.8.1", git = "https://github.com/zackees/running-process.git", rev = "359b4e3d92660e54eb9b16b6e83de8ce26932ff8" } object = { version = "0.36", default-features = false, features = ["read", "std", "elf", "write"] } -rusqlite = { version = "0.31", features = ["bundled"] } +rusqlite = { version = "0.32", features = ["bundled"] } shell-words = "1" bincode = "1" # USB identity data is fetched from FastLED/boards at runtime. Do not add a diff --git a/crates/fbuild-core/Cargo.toml b/crates/fbuild-core/Cargo.toml index 848d52c0e..353ea5413 100644 --- a/crates/fbuild-core/Cargo.toml +++ b/crates/fbuild-core/Cargo.toml @@ -83,3 +83,15 @@ widestring = { workspace = true } tempfile = { workspace = true } # Test-only embedded USB-vendor fixture extraction. Production never links it. tar = { workspace = true } +# FastLED/fbuild#1361: the on-CPU sampler, its frame symbolizer, and the +# off-CPU/async profile pipeline, exercised by tests/cpu_profiling.rs. +# +# It lives here rather than on fbuild-daemon for a linker reason, not a +# taste one. `running-process-probe` pulls crash-handler 0.7 while the +# pinned zccache 1.13.1 pulls 0.6.3, and both export the same unmangled C +# symbols (`ehsetjmp`, `handle_invalid_parameter`, ...). Any binary +# linking both fails with duplicate symbols. fbuild-core is the deepest +# crate that does NOT depend on zccache, so its test binary links exactly +# one copy. The heap half of #1361 stays in fbuild-daemon, which needs no +# probe crate at all. +running-process-probe-daemon = { workspace = true } diff --git a/crates/fbuild-core/tests/cpu_profiling.rs b/crates/fbuild-core/tests/cpu_profiling.rs new file mode 100644 index 000000000..770d47672 --- /dev/null +++ b/crates/fbuild-core/tests/cpu_profiling.rs @@ -0,0 +1,189 @@ +//! On-CPU and off-CPU profiling, exercised end to end (FastLED/fbuild#1361). +//! +//! Two different questions, and fbuild needs both answered: +//! +//! - **on-CPU** — "what is running?" Sampled stacks, then symbolized off the +//! hot path. +//! - **off-CPU** — "what is *waiting*?" This is the mode that matters most for +//! `fbuild-daemon`, which spends nearly all its wall clock blocked on +//! subprocesses, sockets, and the filesystem. A CPU profile is blind to all +//! of that: a request that spent nine seconds waiting and one computing +//! looks, in a CPU profile, like one second of work. +//! +//! These live in `fbuild-core` rather than next to the heap test in +//! `fbuild-daemon` for a linker reason, documented on the dev-dependency in +//! this crate's `Cargo.toml`: `running-process-probe` and the pinned zccache +//! disagree about `crash-handler`, and both export the same unmangled C +//! symbols, so no single binary can link both. + +use std::time::Duration; + +// --------------------------------------------------------------------------- +// On-CPU — sampled stacks plus symbolization +// --------------------------------------------------------------------------- + +/// The frame the on-CPU profile has to find. +/// +/// `#[inline(never)]` because the point is that this symbol survives into the +/// sampled stack; inlined into its caller it would have no address of its own +/// for samples to land on. +#[inline(never)] +fn fbuild_on_cpu_hot_loop(stop: &std::sync::atomic::AtomicBool) -> u64 { + let mut acc = 0u64; + while !stop.load(std::sync::atomic::Ordering::Relaxed) { + for i in 0..4096u64 { + acc = acc.wrapping_add(i).wrapping_mul(31); + } + std::hint::black_box(acc); + } + acc +} + +#[test] +fn on_cpu_profiling_samples_a_busy_thread_and_attributes_the_frames() { + use running_process_probe_daemon::profile::session::{ProfileRequest, ProfileSession}; + use running_process_probe_daemon::profile::symbolize::ModuleResolver; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + // The sampler suspends *sibling* threads, so the work being profiled must + // not sit on the thread calling `run()` — that thread is the profiler. + let stop = Arc::new(AtomicBool::new(false)); + let worker_stop = Arc::clone(&stop); + let worker = std::thread::spawn(move || fbuild_on_cpu_hot_loop(&worker_stop)); + + let session = ProfileSession::new(ProfileRequest { + hz: 99, + duration: Duration::from_millis(400), + }); + let metrics = session.run(); + + stop.store(true, Ordering::Relaxed); + let acc = worker.join().expect("worker thread must not panic"); + std::hint::black_box(acc); + + assert!( + metrics.samples_captured > 0, + "a 400 ms session at 99 Hz over a busy thread must capture samples; \ + captured={} dropped={} threads_seen={}", + metrics.samples_captured, + metrics.samples_dropped, + metrics.threads_seen + ); + assert!( + metrics.threads_seen >= 1, + "at least the busy worker thread must appear in the profile" + ); + + // Symbolization. `ModuleResolver` is deliberately the floor of what can be + // resolved without symbol files: it attributes each address to its owning + // module plus an ASLR-independent offset, and stays honest about the rest + // by naming unresolved frames `module+0xoffset` rather than inventing a + // function name. Asserting DWARF/PDB function names here would be + // asserting on the build's debug-info settings rather than on the + // profiler; #1361 tracks that separately as the release-profile question. + let mut resolver = ModuleResolver::for_current_process() + .expect("module enumeration must work on a supported host"); + assert!( + resolver.module_count() > 0, + "the current process must have at least one loaded module" + ); + let resolved = session.resolve(&mut resolver, metrics); + + let folded = resolved.folded(); + assert!( + !folded.is_empty(), + "resolved samples must fold into at least one stack" + ); + let attributed = folded + .iter() + .flat_map(|(stack, _)| stack.iter()) + .any(|frame| !frame.is_empty()); + assert!( + attributed, + "every folded stack was empty — frames reached no module at all" + ); +} + +// --------------------------------------------------------------------------- +// Off-CPU — the async/waiting pipeline +// --------------------------------------------------------------------------- + +#[test] +fn off_cpu_profiling_ranks_waiting_above_running() { + use running_process_probe_daemon::profile::async_profile::{ + CustomAdapter, TaskSample, profile, to_collapsed, to_pprof, + }; + + // Two tasks with inverted profiles: one waits nine seconds and works for + // one, the other works constantly. In an on-CPU profile the busy task + // dominates and the waiter is invisible. An off-CPU profile has to invert + // that — surfacing the waiter is the entire reason to take one. + let waiting = TaskSample { + spawn_stack: vec![ + "fbuild_daemon::main".to_string(), + "fbuild_daemon::handlers::operations::build".to_string(), + "fbuild_build::compile_many::await_subprocess".to_string(), + ], + idle_nanos: 9_000_000_000, + busy_nanos: 1_000_000_000, + scheduled_nanos: 12_000, + polls: 3, + wakes: 3, + name: "compile-wait".to_string(), + }; + let busy = TaskSample { + spawn_stack: vec![ + "fbuild_daemon::main".to_string(), + "fbuild_daemon::status_manager::tick".to_string(), + ], + idle_nanos: 1_000_000, + busy_nanos: 500_000_000, + scheduled_nanos: 4_000, + polls: 900, + wakes: 900, + name: "status-tick".to_string(), + }; + + let collected = { + let samples = vec![waiting.clone(), busy.clone()]; + let mut adapter = CustomAdapter::new(move |_window| samples.clone()); + profile(&mut adapter, Duration::from_secs(5)).expect("adapter must produce samples") + }; + assert_eq!(collected.len(), 2, "both tasks must survive collection"); + + let pprof = to_pprof(&collected); + assert!( + !pprof.is_empty(), + "off-CPU samples must serialize to a pprof profile" + ); + + let collapsed = to_collapsed(&collected); + assert!( + collapsed.contains("await_subprocess"), + "the waiting task's spawn stack must appear in the off-CPU profile:\n{collapsed}" + ); + + // The ranking, not just the presence. Collapsed output opens on idle time, + // so the waiter has to weigh more than the busy task even though the busy + // task used 500x more CPU. + let waiting_weight = collapsed_weight(&collapsed, "await_subprocess"); + let busy_weight = collapsed_weight(&collapsed, "status_manager::tick"); + assert!( + waiting_weight > busy_weight, + "off-CPU profile must rank waiting above running \ + (waiting={waiting_weight}, busy={busy_weight}):\n{collapsed}" + ); +} + +/// Sum the counts of collapsed lines whose stack mentions `needle`. +/// +/// Collapsed format is `frame;frame;frame ` per line. +fn collapsed_weight(collapsed: &str, needle: &str) -> u64 { + collapsed + .lines() + .filter(|line| line.contains(needle)) + .filter_map(|line| line.rsplit_once(' ')) + .filter_map(|(_stack, count)| count.trim().parse::().ok()) + .sum() +} diff --git a/crates/fbuild-daemon/Cargo.toml b/crates/fbuild-daemon/Cargo.toml index cd9d044e9..17ef2ce56 100644 --- a/crates/fbuild-daemon/Cargo.toml +++ b/crates/fbuild-daemon/Cargo.toml @@ -60,7 +60,7 @@ regex = { workspace = true } async-trait = { workspace = true } tempfile = { workspace = true } -[dependencies.mimalloc] +[dependencies.mimalloc-pprof] workspace = true [dev-dependencies] diff --git a/crates/fbuild-daemon/src/handlers/health.rs b/crates/fbuild-daemon/src/handlers/health.rs index 275b71db1..f96f32c2d 100644 --- a/crates/fbuild-daemon/src/handlers/health.rs +++ b/crates/fbuild-daemon/src/handlers/health.rs @@ -2,7 +2,8 @@ use crate::context::DaemonContext; use crate::models::{ - DaemonInfoResponse, HealthResponse, RootResponse, ShutdownParams, ShutdownResponse, + DaemonInfoResponse, HealthResponse, HeapDumpResponse, RootResponse, ShutdownParams, + ShutdownResponse, }; use axum::Json; use axum::extract::{ConnectInfo, Query, State}; @@ -67,6 +68,58 @@ pub async fn daemon_info(State(ctx): State>) -> Json (StatusCode, Json) { + let was_running = crate::heap_profile::is_enabled(); + if !was_running { + crate::heap_profile::start(DEFAULT_ON_DEMAND_SAMPLE_RATE); + } + + match crate::heap_profile::dump(None) { + Ok(path) => ( + StatusCode::OK, + Json(HeapDumpResponse { + path: Some(path.to_string_lossy().into_owned()), + live_samples: crate::heap_profile::live_sample_count(), + profiling_was_already_running: was_running, + message: if was_running { + "heap snapshot written".to_string() + } else { + "heap snapshot written, but profiling only started with this request — it covers allocations from now on, not the ones already held. Set FBUILD_HEAP_PROFILE=1 before starting the daemon to capture from process start." + .to_string() + }, + }), + ), + Err(error) => ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(HeapDumpResponse { + path: None, + live_samples: 0, + profiling_was_already_running: was_running, + message: format!("heap dump failed: {error}"), + }), + ), + } +} + +/// Sample rate used when a dump is requested on a daemon that was not started +/// with profiling on. 64 KiB is finer than the 512 KiB default: by this point +/// someone is actively chasing something, and the extra resolution is worth +/// more than the overhead. +const DEFAULT_ON_DEMAND_SAMPLE_RATE: usize = 64 * 1024; + /// POST /api/daemon/shutdown pub async fn shutdown( State(ctx): State>, diff --git a/crates/fbuild-daemon/src/heap_profile.rs b/crates/fbuild-daemon/src/heap_profile.rs new file mode 100644 index 000000000..cf9ba00b9 --- /dev/null +++ b/crates/fbuild-daemon/src/heap_profile.rs @@ -0,0 +1,190 @@ +//! Sampled heap profiling for the long-lived daemon (FastLED/fbuild#1361). +//! +//! # Why this exists +//! +//! `fbuild-daemon` is the one fbuild process that outlives a build. It holds +//! caches, device state, and broadcast buffers, so it is where a slow leak is +//! both most likely and least visible — FastLED/fbuild#1360 caught it at +//! ~3.9 GB of resident memory while *idle*, still climbing, until the health +//! probe failed and every build started reporting a compile error. +//! +//! The allocator underneath is `mimalloc-pprof`: the same mimalloc the daemon +//! already used, with a sampled heap profiler attached. That profiler is +//! dormant until something starts it, which is what makes it safe to ship +//! unconditionally rather than behind a build feature. A profiler that has to +//! be compiled in specially is never compiled in on the machine where the leak +//! actually reproduces — and #1360 was found on a real Windows box, twice in +//! one session, with no minimal reproduction. +//! +//! # Two ways in, because leaks are found at two different times +//! +//! - [`start_from_env`] reads `FBUILD_HEAP_PROFILE` at startup. Startup and +//! static-initialization allocations are only visible this way, because +//! anything allocated before the profiler starts is untracked. +//! - [`dump`] can be called on a daemon that is *already* wedged, over HTTP, +//! without restarting it. This matters more than it looks: restarting the +//! daemon destroys the leak, so a restart-only profiler cannot answer the +//! question it exists to answer. +//! +//! Snapshots are pprof `profile.proto` — the same format `go tool pprof` and +//! every flame-graph viewer already read, so nothing here needs a bespoke +//! decoder. + +use std::path::{Path, PathBuf}; + +/// Environment variable that turns heap profiling on at daemon startup. +/// +/// Set to `1` (or any value other than `0`/empty) before the daemon starts. +/// The value may also be a decimal sample rate in bytes, e.g. +/// `FBUILD_HEAP_PROFILE=65536` to sample roughly every 64 KiB allocated. +pub const HEAP_PROFILE_ENV: &str = "FBUILD_HEAP_PROFILE"; + +/// Sample rate used when [`HEAP_PROFILE_ENV`] is set to a plain truthy value. +/// +/// 512 KiB is `mimalloc-pprof`'s own default. Sampling is statistical, so a +/// finer rate buys resolution with proportional overhead; a leak large enough +/// to wedge a daemon is visible at this rate. +const DEFAULT_SAMPLE_RATE: usize = 512 * 1024; + +/// Directory under the dev/prod-isolated fbuild root where dumps are written. +const DUMP_SUBDIR: &str = "heap-profiles"; + +/// Start the profiler if [`HEAP_PROFILE_ENV`] asks for it. +/// +/// Returns the sample rate actually used, or `None` when profiling was not +/// requested or the profiler was already running. Call this as early in +/// `main` as possible — allocations made before it are invisible to every +/// later snapshot. +pub fn start_from_env() -> Option { + let requested = std::env::var(HEAP_PROFILE_ENV).ok()?; + let rate = sample_rate_from(&requested)?; + if mimalloc_pprof::prof::start(rate) { + Some(rate) + } else { + None + } +} + +/// Parse the env var into a sample rate. +/// +/// Separated from [`start_from_env`] so the parsing contract is testable +/// without touching process-wide profiler state. +fn sample_rate_from(value: &str) -> Option { + let value = value.trim(); + match value { + "" | "0" | "false" | "off" => None, + "1" | "true" | "on" => Some(DEFAULT_SAMPLE_RATE), + other => other.parse::().ok().filter(|rate| *rate > 0), + } +} + +/// Whether the profiler is currently sampling. +pub fn is_enabled() -> bool { + mimalloc_pprof::prof::is_enabled() +} + +/// Start profiling at `sample_rate` bytes, for a daemon that is already up. +/// +/// Returns `false` when a session was already running — its rate stays in +/// effect rather than being silently replaced. +pub fn start(sample_rate: usize) -> bool { + mimalloc_pprof::prof::start(sample_rate) +} + +/// Stop sampling. Retained samples stay dumpable. +pub fn stop() { + mimalloc_pprof::prof::stop(); +} + +/// Where [`dump`] writes, absent an explicit path. +/// +/// Routed through `fbuild_paths` so dumps land under the same dev/prod +/// isolation as everything else the daemon writes, rather than the CWD of +/// whoever happened to start it. +pub fn default_dump_dir() -> PathBuf { + fbuild_paths::temp_subdir(DUMP_SUBDIR) +} + +/// Write a pprof `profile.proto` snapshot of the live heap. +/// +/// `path` overrides the destination; otherwise the snapshot lands in +/// [`default_dump_dir`] under a name carrying the daemon's PID, so repeated +/// dumps from one daemon do not overwrite each other and dumps from +/// different daemons do not collide. +/// +/// Returns the path written. Dumping while the profiler is stopped produces +/// a valid but empty profile, so the caller gets a file either way and the +/// emptiness is visible in the profile itself rather than as an error. +pub fn dump(path: Option<&Path>) -> std::io::Result { + let target = match path { + Some(explicit) => explicit.to_path_buf(), + None => { + let dir = default_dump_dir(); + std::fs::create_dir_all(&dir)?; + dir.join(format!("heap-{}.pb", std::process::id())) + } + }; + mimalloc_pprof::prof::dump_proto_file(&target)?; + Ok(target) +} + +/// Live-sample counters, for a health endpoint that wants to report growth +/// without serializing a whole profile. +/// +/// `live_samples` is the sampled count; `live_bytes` the sampled byte total. +/// Both are statistical — the exact allocator counters they are derived from +/// are what make an assertion on a sampled profile meaningful. +pub fn live_sample_count() -> usize { + mimalloc_pprof::prof::stats().live_samples +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn env_values_that_mean_off_do_not_start_a_profiler() { + for value in ["", "0", "false", "off", " "] { + assert_eq!( + sample_rate_from(value), + None, + "{value:?} must not enable profiling" + ); + } + } + + #[test] + fn truthy_env_values_select_the_default_rate() { + for value in ["1", "true", "on", " on "] { + assert_eq!( + sample_rate_from(value), + Some(DEFAULT_SAMPLE_RATE), + "{value:?} must select the default rate" + ); + } + } + + #[test] + fn a_numeric_env_value_is_taken_as_the_sample_rate() { + assert_eq!(sample_rate_from("65536"), Some(65536)); + // A rate of zero would mean "sample everything", which is not what an + // operator typing 0 means — they mean off, and that is the `"0"` case. + assert_eq!(sample_rate_from("00"), None); + assert_eq!(sample_rate_from("not-a-number"), None); + } + + #[test] + fn the_default_dump_dir_is_under_the_isolated_fbuild_root() { + let dir = default_dump_dir(); + assert!( + dir.ends_with(DUMP_SUBDIR), + "dump dir must be the named subdir, got {}", + dir.display() + ); + assert!( + dir.starts_with(fbuild_paths::get_fbuild_root()), + "dump dir must sit under the dev/prod-isolated root, got {}", + dir.display() + ); + } +} diff --git a/crates/fbuild-daemon/src/lib.rs b/crates/fbuild-daemon/src/lib.rs index 1afeba615..2968a4d57 100644 --- a/crates/fbuild-daemon/src/lib.rs +++ b/crates/fbuild-daemon/src/lib.rs @@ -33,6 +33,8 @@ pub mod broker; pub mod context; pub mod device_manager; pub mod handlers; +/// Sampled heap profiling for the long-lived daemon (FastLED/fbuild#1361). +pub mod heap_profile; pub mod lock_models; pub mod log_layer; pub mod models; diff --git a/crates/fbuild-daemon/src/main.rs b/crates/fbuild-daemon/src/main.rs index 9d4a40231..b2bee8761 100644 --- a/crates/fbuild-daemon/src/main.rs +++ b/crates/fbuild-daemon/src/main.rs @@ -1,5 +1,10 @@ +// The same mimalloc the daemon has always used, plus a sampled heap +// profiler that stays dormant until started (FastLED/fbuild#1361). +// Unconditional rather than feature-gated: a profiler compiled out of +// the shipped binary is never present on the machine where a slow leak +// reproduces, which is exactly how #1360 was found. #[global_allocator] -static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; +static GLOBAL: mimalloc_pprof::MiMalloc = mimalloc_pprof::MiMalloc; use axum::Router; use axum::routing::{get, post}; @@ -98,6 +103,17 @@ async fn main() { .with(BroadcastLogLayer::new(log_tx)) .init(); + // FastLED/fbuild#1361 — start sampling before any heavy init when the + // operator asked for it. Anything allocated before this point is + // invisible to every later snapshot, so "as early as the logger" is + // the latest this can usefully go. + if let Some(rate) = fbuild_daemon::heap_profile::start_from_env() { + tracing::info!( + "heap profiling enabled, sampling every ~{rate} bytes; \ + dump with POST /api/daemon/heap-dump" + ); + } + tracing::info!("fbuild daemon starting on port {}", port); // FastLED/fbuild#1010 — bind the endpoint BEFORE any heavy init (the @@ -201,6 +217,10 @@ async fn main() { .route("/health", get(health::health_check)) .route("/api/daemon/info", get(health::daemon_info)) .route("/api/daemon/shutdown", post(health::shutdown)) + // FastLED/fbuild#1361 — obtainable from a daemon that is already + // misbehaving. Restarting to enable a profiler would destroy the + // leak being investigated, which is what made #1360 hard to chase. + .route("/api/daemon/heap-dump", post(health::heap_dump)) .route("/api/build", post(operations::build)) .route("/api/deploy", post(operations::deploy)) .route("/api/monitor", post(operations::monitor)) diff --git a/crates/fbuild-daemon/src/models.rs b/crates/fbuild-daemon/src/models.rs index 75007a387..c12291aa1 100644 --- a/crates/fbuild-daemon/src/models.rs +++ b/crates/fbuild-daemon/src/models.rs @@ -305,6 +305,20 @@ pub struct RootResponse { pub health: String, } +/// Response body for `POST /api/daemon/heap-dump` (FastLED/fbuild#1361). +#[derive(Debug, Serialize, Deserialize)] +pub struct HeapDumpResponse { + /// Where the pprof snapshot was written, absent when the dump failed. + pub path: Option, + /// Sampled live allocations at the moment of the dump. + pub live_samples: usize, + /// False when this request started profiling, which means the snapshot + /// only covers allocations made after it — not memory already held. + pub profiling_was_already_running: bool, + /// Human-readable outcome, including the caveat above when it applies. + pub message: String, +} + /// POST /api/daemon/shutdown #[derive(Debug, Serialize)] pub struct ShutdownResponse { diff --git a/crates/fbuild-daemon/tests/profiling.rs b/crates/fbuild-daemon/tests/profiling.rs new file mode 100644 index 000000000..26a35fd58 --- /dev/null +++ b/crates/fbuild-daemon/tests/profiling.rs @@ -0,0 +1,106 @@ +//! Sampled heap profiling, exercised end to end (FastLED/fbuild#1361). +//! +//! An integration test is its own final executable, so declaring the global +//! allocator here is not incidental — it *is* the downstream linkage contract +//! `fbuild-daemon` relies on. A library cannot choose its consumer's +//! allocator, so if this file did not compile, neither would the daemon. +//! +//! This is the mode #1360 needed and did not have when the daemon reached +//! ~3.9 GB resident while idle: *what is holding this memory?* +//! +//! The on-CPU and off-CPU halves of #1361 live in +//! `fbuild-core/tests/cpu_profiling.rs`, for a linker reason rather than a +//! taste one — see the dev-dependency comment in `fbuild-core/Cargo.toml`. + +use mimalloc_pprof::MiMalloc; + +#[global_allocator] +static ALLOC: MiMalloc = MiMalloc; + +/// Stops the profiler even when an assertion unwinds. +/// +/// The profiler is process-wide state; leaking it enabled would make anything +/// later in this binary run under a sampler it never asked for. +struct ProfilerGuard; + +impl Drop for ProfilerGuard { + fn drop(&mut self) { + mimalloc_pprof::prof::stop(); + } +} + +/// One test, not three, because the profiler is a process-wide singleton. +/// +/// Cargo runs a test binary's cases on parallel threads, so two cases that +/// each start and stop the profiler race: whichever finishes first stops +/// sampling out from under the other, and the loser sees zero live samples. +/// Splitting these would buy nicer names at the cost of a test that fails +/// depending on thread scheduling — so the whole heap contract is asserted in +/// one place, in order. +#[test] +fn heap_profiling_captures_retained_allocations_and_dumps_them_as_pprof() { + if mimalloc_pprof::prof::is_enabled() { + mimalloc_pprof::prof::stop(); + } + // Sample every byte. At the default 512 KiB rate these assertions would be + // probabilistic, and a flaky proof of a profiler is worse than none. + assert!( + fbuild_daemon::heap_profile::start(1), + "heap profiler must start when nothing else holds a session" + ); + let _guard = ProfilerGuard; + assert!( + fbuild_daemon::heap_profile::is_enabled(), + "the daemon helper must report the session it just started" + ); + + // Many small blocks rather than one big one, deliberately. A single + // multi-megabyte `Vec` can be served by mimalloc's large-object path, + // which does not always pass through the sampling hook — a version of + // this test that allocated 4 MiB in one go passed or failed depending on + // arena state. Retaining several thousand 4 KiB blocks is both stable and + // the shape a real leak actually has: many live objects, not one giant + // buffer. Same shape as running-process's own `mimalloc_leaker` fixture. + // + // Held to the end of the test on purpose: a freed allocation is exactly + // what a *live* heap profile must not report, so retention is what makes + // these assertions mean "still held" rather than "allocated at some + // point". + const BLOCKS: usize = 2048; + let retained: Vec> = (0..BLOCKS).map(|_| Box::new([0xA5_u8; 4096])).collect(); + std::hint::black_box(&retained); + + // 1. The allocations are sampled. + let stats = mimalloc_pprof::prof::stats(); + assert!( + stats.live_samples > 0, + "{BLOCKS} retained 4 KiB blocks must appear in the live sample set \ + (live_bytes={}, enabled={})", + stats.live_bytes, + mimalloc_pprof::prof::is_enabled() + ); + assert!( + fbuild_daemon::heap_profile::live_sample_count() > 0, + "the daemon helper must surface the same live sample count" + ); + + // 2. It serializes to pprof in memory — the shape a viewer consumes. + let snapshot = mimalloc_pprof::prof::dump_proto_to_vec(); + assert!( + !snapshot.is_empty(), + "pprof profile.proto snapshot must not be empty" + ); + + // 3. And it can be written to a file from a *running* process. This is + // the half #1360 actually needed: the daemon was already wedged, and + // restarting it to enable a profiler would have destroyed the leak + // being investigated. + let tmp = tempfile::TempDir::new().expect("tempdir"); + let target = tmp.path().join("heap.pb"); + let written = fbuild_daemon::heap_profile::dump(Some(&target)).expect("dump must succeed"); + assert_eq!(written, target); + let bytes = std::fs::read(&written).expect("dump file must be readable"); + assert!(!bytes.is_empty(), "dump file must not be empty"); + + std::hint::black_box(&retained); +} From 7526851c1a8be1eb8f55b15b62c64ade008b1be3 Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 19:45:45 -0700 Subject: [PATCH 2/4] fix(daemon): satisfy the workspace lint gate in the new profiling module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full local `dylint --all` sweep on Windows caught four real violations in `heap_profile.rs` that CI would have rejected — worth recording because none were obvious from reading the code: - `default_dump_dir` and `dump` returned `PathBuf` (`ban_std_pathbuf`). Both now return `NormalizedPath`, which is what the workspace uses for a path that crosses an API boundary. - `std::fs::create_dir_all` sat on a path reachable from an axum handler (`ban_std_fs_in_async`, FastLED/fbuild#844) — it would have blocked a tokio worker. `dump` is now async and uses `fbuild_core::fs::create_dir_all`; the pprof write itself stays sync, since it is a C call into the allocator rather than a filesystem-bound operation. - The module's own unit test compared paths with `Path::starts_with` (`ban_raw_path_prefix_compare`, #952). It now compares through `normalize_for_key`, so a verbatim prefix or case difference cannot make the assertion lie. Sweep is now clean workspace-wide. The one remaining violation is `fbuild-core/src/platform/windows/fs.rs`, which this branch does not touch and which reproduces on `main` — filed as #1359, since the Dylint job runs ubuntu-only and has never compiled that file. Refs FastLED/fbuild#1361 Co-Authored-By: Claude Opus 5 (1M context) --- crates/fbuild-daemon/src/handlers/health.rs | 4 +- crates/fbuild-daemon/src/heap_profile.rs | 48 +++++++++++++-------- crates/fbuild-daemon/tests/profiling.rs | 11 +++-- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/health.rs b/crates/fbuild-daemon/src/handlers/health.rs index f96f32c2d..ba1f68802 100644 --- a/crates/fbuild-daemon/src/handlers/health.rs +++ b/crates/fbuild-daemon/src/handlers/health.rs @@ -87,11 +87,11 @@ pub async fn heap_dump() -> (StatusCode, Json) { crate::heap_profile::start(DEFAULT_ON_DEMAND_SAMPLE_RATE); } - match crate::heap_profile::dump(None) { + match crate::heap_profile::dump(None).await { Ok(path) => ( StatusCode::OK, Json(HeapDumpResponse { - path: Some(path.to_string_lossy().into_owned()), + path: Some(path.display_slash()), live_samples: crate::heap_profile::live_sample_count(), profiling_was_already_running: was_running, message: if was_running { diff --git a/crates/fbuild-daemon/src/heap_profile.rs b/crates/fbuild-daemon/src/heap_profile.rs index cf9ba00b9..9940b312a 100644 --- a/crates/fbuild-daemon/src/heap_profile.rs +++ b/crates/fbuild-daemon/src/heap_profile.rs @@ -30,7 +30,9 @@ //! every flame-graph viewer already read, so nothing here needs a bespoke //! decoder. -use std::path::{Path, PathBuf}; +use std::path::Path; + +use fbuild_core::path::NormalizedPath; /// Environment variable that turns heap profiling on at daemon startup. /// @@ -101,8 +103,8 @@ pub fn stop() { /// Routed through `fbuild_paths` so dumps land under the same dev/prod /// isolation as everything else the daemon writes, rather than the CWD of /// whoever happened to start it. -pub fn default_dump_dir() -> PathBuf { - fbuild_paths::temp_subdir(DUMP_SUBDIR) +pub fn default_dump_dir() -> NormalizedPath { + NormalizedPath::new(fbuild_paths::temp_subdir(DUMP_SUBDIR)) } /// Write a pprof `profile.proto` snapshot of the live heap. @@ -115,25 +117,33 @@ pub fn default_dump_dir() -> PathBuf { /// Returns the path written. Dumping while the profiler is stopped produces /// a valid but empty profile, so the caller gets a file either way and the /// emptiness is visible in the profile itself rather than as an error. -pub fn dump(path: Option<&Path>) -> std::io::Result { +/// +/// Async because it is reached from an axum handler: `create_dir_all` on a +/// tokio worker would block the runtime (FastLED/fbuild#844). +pub async fn dump(path: Option<&Path>) -> std::io::Result { let target = match path { - Some(explicit) => explicit.to_path_buf(), + Some(explicit) => NormalizedPath::new(explicit), None => { let dir = default_dump_dir(); - std::fs::create_dir_all(&dir)?; - dir.join(format!("heap-{}.pb", std::process::id())) + fbuild_core::fs::create_dir_all(dir.as_path()).await?; + NormalizedPath::new( + dir.as_path() + .join(format!("heap-{}.pb", std::process::id())), + ) } }; - mimalloc_pprof::prof::dump_proto_file(&target)?; + // The dump itself is a synchronous C call into the allocator and is not + // filesystem-bound in the way `create_dir_all` is, so it does not need + // offloading. + mimalloc_pprof::prof::dump_proto_file(target.as_path())?; Ok(target) } -/// Live-sample counters, for a health endpoint that wants to report growth +/// Live-sample count, for a health endpoint that wants to report growth /// without serializing a whole profile. /// -/// `live_samples` is the sampled count; `live_bytes` the sampled byte total. -/// Both are statistical — the exact allocator counters they are derived from -/// are what make an assertion on a sampled profile meaningful. +/// Statistical, not exact — the sampler is what makes it cheap enough to +/// leave on. pub fn live_sample_count() -> usize { mimalloc_pprof::prof::stats().live_samples } @@ -177,14 +187,18 @@ mod tests { fn the_default_dump_dir_is_under_the_isolated_fbuild_root() { let dir = default_dump_dir(); assert!( - dir.ends_with(DUMP_SUBDIR), + dir.as_path().ends_with(DUMP_SUBDIR), "dump dir must be the named subdir, got {}", - dir.display() + dir.display_slash() ); + // Compared through `normalize_for_key` rather than `Path::starts_with`: + // the two sides can be spelled differently (verbatim prefix, case) and + // still be the same directory (FastLED/fbuild#952). + let root = fbuild_core::path::normalize_for_key(&fbuild_paths::get_fbuild_root()); + let dir_key = fbuild_core::path::normalize_for_key(dir.as_path()); assert!( - dir.starts_with(fbuild_paths::get_fbuild_root()), - "dump dir must sit under the dev/prod-isolated root, got {}", - dir.display() + dir_key.starts_with(&root), + "dump dir must sit under the dev/prod-isolated root; dir={dir_key} root={root}" ); } } diff --git a/crates/fbuild-daemon/tests/profiling.rs b/crates/fbuild-daemon/tests/profiling.rs index 26a35fd58..8a2074034 100644 --- a/crates/fbuild-daemon/tests/profiling.rs +++ b/crates/fbuild-daemon/tests/profiling.rs @@ -97,9 +97,14 @@ fn heap_profiling_captures_retained_allocations_and_dumps_them_as_pprof() { // being investigated. let tmp = tempfile::TempDir::new().expect("tempdir"); let target = tmp.path().join("heap.pb"); - let written = fbuild_daemon::heap_profile::dump(Some(&target)).expect("dump must succeed"); - assert_eq!(written, target); - let bytes = std::fs::read(&written).expect("dump file must be readable"); + let written = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime") + .block_on(fbuild_daemon::heap_profile::dump(Some(&target))) + .expect("dump must succeed"); + assert_eq!(written.as_path(), target.as_path()); + let bytes = std::fs::read(written.as_path()).expect("dump file must be readable"); assert!(!bytes.is_empty(), "dump file must not be empty"); std::hint::black_box(&retained); From 27fe07c4a71bb0648759fd7eabe13620166f1ead Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 20:05:38 -0700 Subject: [PATCH 3/4] docs(daemon): qualify the heap_profile intra-doc links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RUSTDOCFLAGS="-D warnings" cargo doc` rejected `[`start_from_env`]` and `[`dump`]` with "no item named ... in scope", even though both are `pub` in the same module. The module carried docs in two places at once — an outer `///` on `pub mod heap_profile;` in lib.rs and the file's own `//!` block — and the merge left the inner links resolving against the wrong scope. Dropped the redundant outer doc (the file's `//!` is the real one) and qualified the links with `self::`, which is unambiguous regardless of how the two blocks merge. Verified: `RUSTDOCFLAGS="-D warnings" soldr cargo doc --workspace --no-deps` is clean. Refs FastLED/fbuild#1361 Co-Authored-By: Claude Opus 5 (1M context) --- crates/fbuild-daemon/src/heap_profile.rs | 14 +++++++------- crates/fbuild-daemon/src/lib.rs | 1 - 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/crates/fbuild-daemon/src/heap_profile.rs b/crates/fbuild-daemon/src/heap_profile.rs index 9940b312a..c52508b81 100644 --- a/crates/fbuild-daemon/src/heap_profile.rs +++ b/crates/fbuild-daemon/src/heap_profile.rs @@ -18,10 +18,10 @@ //! //! # Two ways in, because leaks are found at two different times //! -//! - [`start_from_env`] reads `FBUILD_HEAP_PROFILE` at startup. Startup and +//! - [`self::start_from_env`] reads `FBUILD_HEAP_PROFILE` at startup. Startup and //! static-initialization allocations are only visible this way, because //! anything allocated before the profiler starts is untracked. -//! - [`dump`] can be called on a daemon that is *already* wedged, over HTTP, +//! - [`self::dump`] can be called on a daemon that is *already* wedged, over HTTP, //! without restarting it. This matters more than it looks: restarting the //! daemon destroys the leak, so a restart-only profiler cannot answer the //! question it exists to answer. @@ -41,7 +41,7 @@ use fbuild_core::path::NormalizedPath; /// `FBUILD_HEAP_PROFILE=65536` to sample roughly every 64 KiB allocated. pub const HEAP_PROFILE_ENV: &str = "FBUILD_HEAP_PROFILE"; -/// Sample rate used when [`HEAP_PROFILE_ENV`] is set to a plain truthy value. +/// Sample rate used when [`self::HEAP_PROFILE_ENV`] is set to a plain truthy value. /// /// 512 KiB is `mimalloc-pprof`'s own default. Sampling is statistical, so a /// finer rate buys resolution with proportional overhead; a leak large enough @@ -51,7 +51,7 @@ const DEFAULT_SAMPLE_RATE: usize = 512 * 1024; /// Directory under the dev/prod-isolated fbuild root where dumps are written. const DUMP_SUBDIR: &str = "heap-profiles"; -/// Start the profiler if [`HEAP_PROFILE_ENV`] asks for it. +/// Start the profiler if [`self::HEAP_PROFILE_ENV`] asks for it. /// /// Returns the sample rate actually used, or `None` when profiling was not /// requested or the profiler was already running. Call this as early in @@ -69,7 +69,7 @@ pub fn start_from_env() -> Option { /// Parse the env var into a sample rate. /// -/// Separated from [`start_from_env`] so the parsing contract is testable +/// Separated from [`self::start_from_env`] so the parsing contract is testable /// without touching process-wide profiler state. fn sample_rate_from(value: &str) -> Option { let value = value.trim(); @@ -98,7 +98,7 @@ pub fn stop() { mimalloc_pprof::prof::stop(); } -/// Where [`dump`] writes, absent an explicit path. +/// Where [`self::dump`] writes, absent an explicit path. /// /// Routed through `fbuild_paths` so dumps land under the same dev/prod /// isolation as everything else the daemon writes, rather than the CWD of @@ -110,7 +110,7 @@ pub fn default_dump_dir() -> NormalizedPath { /// Write a pprof `profile.proto` snapshot of the live heap. /// /// `path` overrides the destination; otherwise the snapshot lands in -/// [`default_dump_dir`] under a name carrying the daemon's PID, so repeated +/// [`self::default_dump_dir`] under a name carrying the daemon's PID, so repeated /// dumps from one daemon do not overwrite each other and dumps from /// different daemons do not collide. /// diff --git a/crates/fbuild-daemon/src/lib.rs b/crates/fbuild-daemon/src/lib.rs index 2968a4d57..e198cd9b2 100644 --- a/crates/fbuild-daemon/src/lib.rs +++ b/crates/fbuild-daemon/src/lib.rs @@ -33,7 +33,6 @@ pub mod broker; pub mod context; pub mod device_manager; pub mod handlers; -/// Sampled heap profiling for the long-lived daemon (FastLED/fbuild#1361). pub mod heap_profile; pub mod lock_models; pub mod log_layer; From 17e919360ccb7bc21450168bf9d6fc9c5c592d68 Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 20:31:16 -0700 Subject: [PATCH 4/4] fix(daemon): gate heap-dump to loopback, make dump names unique, profile from process start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three CodeRabbit findings on #1365, all correct: - **The endpoint was reachable off-box.** The daemon binds `0.0.0.0`, and `heap-dump` is not a read: it can switch process-wide profiling on and make the daemon serialize its whole heap on demand. From a network that is both a denial-of-service lever and a way to read allocation shapes off someone else's machine. Nothing about a profiling dump needs to cross a network boundary, so non-loopback callers now get a flat 403 rather than a rate limit. - **Default dump names overwrote each other.** `heap-{pid}.pb` meant the second dump destroyed the first — in a workflow whose entire purpose is *comparing* snapshots over time. Names are now `heap-{pid}-{millis}-{seq}.pb`: the PID separates daemons, the stamp orders snapshots and survives a recycled PID after a restart, and the sequence keeps two dumps inside one millisecond apart. Covered by a test that generates 64 back-to-back names and asserts they are all distinct. - **Profiling started too late to mean what its docs claimed.** The call sat after `Args::parse`, containment setup, `BroadcastHub::new`, and tracing init, so allocations from all of those were permanently unattributable — while the module doc said startup allocations were visible. It is now the first statement in `main`, with the rate logged later once tracing exists to log it to. Refs FastLED/fbuild#1361 Co-Authored-By: Claude Opus 5 (1M context) --- crates/fbuild-daemon/src/handlers/health.rs | 24 +++++++++++- crates/fbuild-daemon/src/heap_profile.rs | 43 +++++++++++++++++++-- crates/fbuild-daemon/src/main.rs | 18 +++++---- 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/health.rs b/crates/fbuild-daemon/src/handlers/health.rs index ba1f68802..4c1339c00 100644 --- a/crates/fbuild-daemon/src/handlers/health.rs +++ b/crates/fbuild-daemon/src/handlers/health.rs @@ -81,7 +81,29 @@ pub async fn daemon_info(State(ctx): State>) -> Json (StatusCode, Json) { +pub async fn heap_dump( + ConnectInfo(peer): ConnectInfo, +) -> (StatusCode, Json) { + // Loopback only. The daemon binds 0.0.0.0 (see `main.rs`), and this + // endpoint is not a read: it can switch process-wide profiling on and make + // the daemon serialize its whole heap on demand. Reachable from off-box + // that is both a denial-of-service lever and a way to extract allocation + // shapes from someone else's machine. Nothing about a profiling dump needs + // to cross a network boundary, so the check is a flat refusal rather than + // a rate limit. + if !peer.ip().is_loopback() { + tracing::warn!(peer = %peer, "heap-dump refused: non-loopback caller"); + return ( + StatusCode::FORBIDDEN, + Json(HeapDumpResponse { + path: None, + live_samples: 0, + profiling_was_already_running: crate::heap_profile::is_enabled(), + message: "heap-dump is loopback-only".to_string(), + }), + ); + } + let was_running = crate::heap_profile::is_enabled(); if !was_running { crate::heap_profile::start(DEFAULT_ON_DEMAND_SAMPLE_RATE); diff --git a/crates/fbuild-daemon/src/heap_profile.rs b/crates/fbuild-daemon/src/heap_profile.rs index c52508b81..301003d1a 100644 --- a/crates/fbuild-daemon/src/heap_profile.rs +++ b/crates/fbuild-daemon/src/heap_profile.rs @@ -126,10 +126,7 @@ pub async fn dump(path: Option<&Path>) -> std::io::Result { None => { let dir = default_dump_dir(); fbuild_core::fs::create_dir_all(dir.as_path()).await?; - NormalizedPath::new( - dir.as_path() - .join(format!("heap-{}.pb", std::process::id())), - ) + NormalizedPath::new(dir.as_path().join(next_dump_name())) } }; // The dump itself is a synchronous C call into the allocator and is not @@ -139,6 +136,26 @@ pub async fn dump(path: Option<&Path>) -> std::io::Result { Ok(target) } +/// Monotonic counter behind [`self::next_dump_name`]. +static DUMP_SEQUENCE: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + +/// A dump file name that no earlier dump can have used. +/// +/// Three parts, each earning its place: the PID separates daemons, the +/// millisecond stamp orders snapshots and survives a PID being recycled by a +/// restarted daemon, and the sequence number keeps two dumps inside the same +/// millisecond apart. Overwriting matters here more than it usually does — +/// leak investigation is *comparing* snapshots over time, so a name that +/// clobbers the previous one destroys the evidence being gathered. +fn next_dump_name() -> String { + let seq = DUMP_SEQUENCE.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let millis = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|since| since.as_millis()) + .unwrap_or(0); + format!("heap-{}-{millis}-{seq}.pb", std::process::id()) +} + /// Live-sample count, for a health endpoint that wants to report growth /// without serializing a whole profile. /// @@ -183,6 +200,24 @@ mod tests { assert_eq!(sample_rate_from("not-a-number"), None); } + #[test] + fn successive_dump_names_never_collide() { + // A leak investigation compares snapshots taken minutes apart. A name + // that reuses the previous one deletes the evidence, so this is the + // property that matters, not the exact format. + let names: Vec = (0..64).map(|_| next_dump_name()).collect(); + let unique: std::collections::HashSet<&String> = names.iter().collect(); + assert_eq!( + unique.len(), + names.len(), + "dump names must be unique even when generated back to back: {names:?}" + ); + assert!( + names.iter().all(|name| name.ends_with(".pb")), + "dump names must keep the .pb extension" + ); + } + #[test] fn the_default_dump_dir_is_under_the_isolated_fbuild_root() { let dir = default_dump_dir(); diff --git a/crates/fbuild-daemon/src/main.rs b/crates/fbuild-daemon/src/main.rs index b2bee8761..4b67eae4f 100644 --- a/crates/fbuild-daemon/src/main.rs +++ b/crates/fbuild-daemon/src/main.rs @@ -40,6 +40,14 @@ struct Args { #[tokio::main] async fn main() { + // FastLED/fbuild#1361 — first statement in the process, ahead of argument + // parsing, containment setup, the broadcast hub, and the tracing + // subscriber. Every allocation made before the profiler starts is + // invisible to every later snapshot, so anything this call sits behind is + // permanently unattributable. The rate is logged further down, once + // tracing exists to log it to. + let heap_profile_rate = fbuild_daemon::heap_profile::start_from_env(); + let args = Args::parse(); if args.dev { @@ -103,14 +111,10 @@ async fn main() { .with(BroadcastLogLayer::new(log_tx)) .init(); - // FastLED/fbuild#1361 — start sampling before any heavy init when the - // operator asked for it. Anything allocated before this point is - // invisible to every later snapshot, so "as early as the logger" is - // the latest this can usefully go. - if let Some(rate) = fbuild_daemon::heap_profile::start_from_env() { + if let Some(rate) = heap_profile_rate { tracing::info!( - "heap profiling enabled, sampling every ~{rate} bytes; \ - dump with POST /api/daemon/heap-dump" + "heap profiling enabled from process start, sampling every ~{rate} \ + bytes; dump with POST /api/daemon/heap-dump" ); }