flowey: memoize expensive steps across local runs - #4055
Conversation
flowey's DAG prunes work by reachability, never freshness: every step body runs unconditionally on every invocation. That is fine for cheap steps, but flowey also does a fair amount of expensive post-processing whose output is identical run after run - splitting debug info out of a multi-gigabyte binary, extracting large archives, fetching release assets. On a local vmm-tests-run against a debug build, redoing that work dominates the inner loop even when nothing has changed. Add a key-addressed memoization store under flowey's persistent dir. A step declares a key covering everything its results depend on, and emit_memoized_rust_step either replays the directory a previous run produced or runs the body to build a new one. The body stays an ordinary flowey step: it works in its working directory, which is simply pointed at a staging area that gets installed into the store on success. The step yields that directory and callers derive whatever paths they need from it, so an entry is just its contents and there is no separate schema of declared outputs to keep in sync. Keys stamp input files by length and mtime rather than hashing contents - the same trade cargo and ninja make. A rebuild producing byte-identical output will miss, but keys stay cheap on gigabyte inputs. Key material drawn from the filesystem has to be sampled at runtime rather than at flow-build time, since the pipeline may be generated on a different machine than the one that runs it; the API takes an otherwise-unused runtime handle so that sampling early fails to compile. Entries are installed atomically and stamped as a whole, so an interrupted run cannot leave a partial entry behind and an externally modified one is evicted rather than served forever. The store is size-capped and collected at the end of each local run, with cargo xflowey-cache for inspection and manual cleanup. Where no persistent dir is available - CI, typically - memoized steps degrade into plain steps, and FLOWEY_NO_MEMO=1 disables memoization entirely. Converting the release-download node fixes a latent bug along the way. It previously downloaded straight to its final cache path, so an interrupted download left a truncated file that every later run treated as a permanent cache hit. Staging and atomic install make that unrepresentable. Separately, direct_run_do_work moved the process working directory as it ran each step but never put it back, so its caller resolved relative paths against whatever directory the last step happened to leave behind. It now restores the directory on every exit path, which also fixes post-run cleanup that had been silently doing nothing.
There was a problem hiding this comment.
Pull request overview
This PR adds a persistent, key-addressed memoization store to Flowey so expensive local steps (archive extraction, release asset downloads, debug symbol splitting) can be skipped across runs when inputs/tools haven’t changed. It also introduces a CLI for inspecting/cleaning the store and fixes local direct-run cwd handling.
Changes:
- Introduce
flowey_core::node::memo(memo store + keying/stamping + GC) andNodeCtx::emit_memoized_rust_step. - Convert several expensive nodes (debug-info splitting, azcopy extraction, archive extractions, GH release downloads) to use the memo store / atomic staging.
- Add
cargo xflowey-cachecommand and run best-effort memo-store GC after local runs.
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| flowey/flowey_lib_hvlite/src/run_split_debug_info.rs | Memoize objcopy-based debug-info splitting and resolve outputs from memoized dir |
| flowey/flowey_lib_hvlite/src/resolve_openvmm_test_virtio_win.rs | Switch archive extraction to use memo store handle |
| flowey/flowey_lib_hvlite/src/resolve_openvmm_test_linux_kernel.rs | Switch archive extraction to use memo store handle |
| flowey/flowey_lib_hvlite/src/resolve_openvmm_test_initrd.rs | Switch archive extraction to use memo store handle |
| flowey/flowey_lib_hvlite/src/resolve_openvmm_qemu.rs | Switch archive extraction to use memo store handle |
| flowey/flowey_lib_hvlite/src/resolve_openvmm_deps.rs | Switch deps archive extraction to use memo store handle |
| flowey/flowey_lib_common/src/download_gh_release.rs | Use memo store per-asset entries for atomicity + reuse; adjust download batching |
| flowey/flowey_lib_common/src/download_azcopy.rs | Memoize azcopy extraction and resolve path from memoized dir |
| flowey/flowey_lib_common/src/cache.rs | Rework cache restore/save to use memo store rather than local “cache_keys” file |
| flowey/flowey_lib_common/src/_util/extract.rs | Reimplement extract helpers on top of memo store (keyed by version + archive stamp) |
| flowey/flowey_core/src/node/memo.rs | New memoization store implementation (keys, stamps, atomic install, GC, CLI helpers) |
| flowey/flowey_core/src/node.rs | Expose memo API + add memo_store() and emit_memoized_rust_step() |
| flowey/flowey_core/Cargo.toml | Add deps needed for memo store (log/sha2/which) |
| flowey/flowey_cli/src/pipeline_resolver/direct_run.rs | Restore cwd via guard and add memo-store GC hook for local runs |
| flowey/flowey_cli/src/cli/mod.rs | Add cache subcommand wiring |
| flowey/flowey_cli/src/cli/cache.rs | New CLI for memo store stats/gc/clear |
| ci-flowey/openvmm-pr.yaml | Regenerated pipeline steps to match new node step structure |
| Cargo.lock | Lockfile updates for new dependencies |
| .cargo/config.toml | Add xflowey-cache cargo alias |
Comments suppressed due to low confidence (3)
flowey/flowey_core/src/node.rs:1830
memo_store()is also emitted as a minor step and usesunwrap()when opening/creating the store. Opening can fail for normal reasons (permissions, invalid path), and panicking here will be much harder to diagnose than returning an error from the step.
self.emit_minor_rust_stepv("🌼 Open memoization store", |ctx| {
let path = path.claim(ctx);
|rt| memo::MemoStore::open(rt.read(path).join(memo::MEMO_STORE_DIR_NAME)).unwrap()
}),
flowey/flowey_lib_hvlite/src/run_split_debug_info.rs:129
in_bin.file_name().unwrap()can panic if the provided path doesn't end in a normal filename (e.g. it is a directory, ends in.., or is otherwise malformed). Sincein_bincomes from runtime configuration/vars, prefer returning an error with context instead of panicking.
This issue also appears on line 151 of the same file.
let in_bin = rt.read(in_bin);
let output = rt.sh.current_dir().join(in_bin.file_name().unwrap());
flowey::shell_cmd!(rt, "{objcopy_bin} --only-keep-debug {in_bin} {output}.dbg")
flowey/flowey_lib_hvlite/src/run_split_debug_info.rs:160
- This step is emitted as
emit_minor_rust_stepbut does fallible work (file_name().unwrap()) and would panic on an unexpectedin_binvalue. Consider making it a regularemit_rust_stepso it can return an error instead of panicking.
ctx.emit_minor_rust_step("resolve split debug info paths", |ctx| {
let dir = dir.claim(ctx);
let in_bin = in_bin.claim(ctx);
let out_bin = out_bin.claim(ctx);
let out_dbg_info = out_dbg_info.claim(ctx);
move |rt| {
let stripped = rt.read(dir).join(rt.read(in_bin).file_name().unwrap());
rt.write(out_dbg_info, &stripped.with_extension("dbg"));
rt.write(out_bin, &stripped);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
flowey/flowey_core/src/node.rs:1791
persistent_dir()now usesemit_minor_rust_stepvandcreate_dir_all(...).unwrap(), which will panic on any I/O error (permissions, read-only FS, etc.). This should remain fallible and propagate an error instead of crashing the whole flowey run.
let dir = rt.read(path).join(folder_name);
fs_err::create_dir_all(&dir).unwrap();
dir
flowey/flowey_cli/src/pipeline_resolver/direct_run.rs:59
gc_memo_store()is only called afterres?, so a failed local run skips memo-store GC (contradicting the PR description that GC happens at the end of each local run). Since GC is already best-effort, it can run regardless ofdirect_run_do_worksuccess/failure.
res?;
gc_memo_store(&persist_dir);
flowey/flowey_lib_hvlite/src/run_split_debug_info.rs:129
- These
file_name().unwrap()calls will panic ifin_bindoesn't have a final path component (e.g. a directory path or a path ending in..). Even if unlikely, this is in a runtime step and should be a normal error instead of aborting the whole run.
This issue also appears on line 151 of the same file.
let in_bin = rt.read(in_bin);
let output = rt.sh.current_dir().join(in_bin.file_name().unwrap());
flowey::shell_cmd!(rt, "{objcopy_bin} --only-keep-debug {in_bin} {output}.dbg")
flowey/flowey_lib_hvlite/src/run_split_debug_info.rs:160
- The follow-up step uses
file_name().unwrap()and is emitted as a minor (infallible) step, so a badin_binpath will panic instead of surfacing as a step error. Consider making this a fallible step and returning a clear error ifin_bindoesn't name a file.
ctx.emit_minor_rust_step("resolve split debug info paths", |ctx| {
let dir = dir.claim(ctx);
let in_bin = in_bin.claim(ctx);
let out_bin = out_bin.claim(ctx);
let out_dbg_info = out_dbg_info.claim(ctx);
move |rt| {
let stripped = rt.read(dir).join(rt.read(in_bin).file_name().unwrap());
rt.write(out_dbg_info, &stripped.with_extension("dbg"));
rt.write(out_bin, &stripped);
}
flowey/flowey_core/src/node/memo.rs:808
- When an entry's
memo.jsonis unreadable,entries()currently reportssize_bytes = 0. That undercountsstats()and can preventgc()from evicting oversized stores (since eviction uses the summedsize_bytes). Consider falling back to computing the directory size when metadata can't be parsed.
let dir = e.path();
let meta = fs_err::read(dir.join(META_FILE))
.ok()
.and_then(|raw| serde_json::from_slice::<MemoEntryMeta>(&raw).ok());
out.push(MemoEntryInfo {
hash: e.file_name().to_string_lossy().into_owned(),
size_bytes: meta.as_ref().map(|m| m.size_bytes).unwrap_or(0),
last_used_unix_secs: self.last_used(&dir),
meta,
});
flowey/flowey_core/src/node.rs:1831
memo_store()unwrapsMemoStore::open(...), which will panic if the store can't be created/opened (e.g. permissions, bad path). This should return ananyhow::Resultviaemit_rust_stepvso failures surface as step errors instead of a process abort.
self.emit_minor_rust_stepv("🌼 Open memoization store", |ctx| {
let path = path.claim(ctx);
|rt| memo::MemoStore::open(rt.read(path).join(memo::MEMO_STORE_DIR_NAME)).unwrap()
}),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
flowey/flowey_lib_hvlite/src/run_split_debug_info.rs:130
- This step assumes
in_bin.file_name()is alwaysSomeand usesunwrap(). Ifin_binever ends with..or is otherwise not a normal file path, this will panic. Since this is runtime path data, prefer returning an error instead of panicking.
This issue also appears on line 151 of the same file.
let in_bin = rt.read(in_bin);
let output = rt.sh.current_dir().join(in_bin.file_name().unwrap());
flowey::shell_cmd!(rt, "{objcopy_bin} --only-keep-debug {in_bin} {output}.dbg")
.run()?;
flowey/flowey_lib_hvlite/src/run_split_debug_info.rs:160
resolve split debug info pathsis emitted as a minor step (cannot fail) but usesfile_name().unwrap(), so a malformed path would panic. Since this is fallible, emit a regular rust step and return a proper error.
ctx.emit_minor_rust_step("resolve split debug info paths", |ctx| {
let dir = dir.claim(ctx);
let in_bin = in_bin.claim(ctx);
let out_bin = out_bin.claim(ctx);
let out_dbg_info = out_dbg_info.claim(ctx);
move |rt| {
let stripped = rt.read(dir).join(rt.read(in_bin).file_name().unwrap());
rt.write(out_dbg_info, &stripped.with_extension("dbg"));
rt.write(out_bin, &stripped);
}
flowey/flowey_core/src/node.rs:1790
persistent_dir()now usesemit_minor_rust_stepvandunwrap()forcreate_dir_all. Directory creation can fail (permissions, disk full), and panicking here crashes the run instead of returning an error. Since this is fallible I/O, this should be a regular rust step that propagatesanyhow::Result.
self.emit_minor_rust_stepv("🌼 Create persistent store dir", |ctx| {
let path = path.claim(ctx);
|rt| {
let dir = rt.read(path).join(folder_name);
fs_err::create_dir_all(&dir).unwrap();
flowey/flowey_core/src/node.rs:1830
memo_store()opens/creates the store directory but usesemit_minor_rust_stepv+unwrap(). Opening/creating the memo store can fail and should propagate an error rather than panicking (especially since this runs in many pipelines).
self.emit_minor_rust_stepv("🌼 Open memoization store", |ctx| {
let path = path.claim(ctx);
|rt| memo::MemoStore::open(rt.read(path).join(memo::MEMO_STORE_DIR_NAME)).unwrap()
}),
flowey/flowey_lib_common/src/cache.rs:32
- Typo/wording in the doc comment: "Github" should be "GitHub", and "tasks's" should be "task's".
//! - Github: use the cache tasks's web UI to manage cache entries
| max_size: Option<String>, | ||
| }, | ||
| /// Remove every entry from the store. | ||
| Clear, |
There was a problem hiding this comment.
IIRC we have an xtask clean command that deletes a bunch of stuff cargo clean can't see, like flowey state. Should make sure it's catching this (I think it just deletes the whole persistent dir but should double check)
|
|
||
| Some( | ||
| self.emit_rust_stepv("🌼 Create persistent store dir", |ctx| { | ||
| self.emit_minor_rust_stepv("🌼 Create persistent store dir", |ctx| { |
There was a problem hiding this comment.
Didn't you tell me not to do exactly this in a past pr lol
| /// Tmp dirs older than this are assumed to be leftovers from a crashed (or | ||
| /// memoization-disabled) run, and are cleaned up by [`MemoStore::gc`]. | ||
| const TMP_MAX_AGE_SECS: u64 = 24 * 60 * 60; | ||
|
|
||
| /// Entries touched more recently than this are never evicted, since a | ||
| /// concurrently-running flowey may be holding paths into them. | ||
| const EVICTION_GRACE_SECS: u64 = 60 * 60; |
There was a problem hiding this comment.
Yay, cache coherency! Everyone's favorite source of heisenbugs lol
smalis-msft
left a comment
There was a problem hiding this comment.
While this is neat, how much time does it tend to save? It's a significant amount of extra complexity.
flowey's DAG prunes work by reachability, never freshness: every step body runs unconditionally on every invocation. That is fine for cheap steps, but flowey also does a fair amount of expensive post-processing whose output is identical run after run - splitting debug info out of a multi-gigabyte binary, extracting large archives, fetching release assets. On a local vmm-tests-run against a debug build, redoing that work dominates the inner loop even when nothing has changed.
Add a key-addressed memoization store under flowey's persistent dir. A step declares a key covering everything its results depend on, and emit_memoized_rust_step either replays the directory a previous run produced or runs the body to build a new one. The body stays an ordinary flowey step: it works in its working directory, which is simply pointed at a staging area that gets installed into the store on success. The step yields that directory and callers derive whatever paths they need from it, so an entry is just its contents and there is no separate schema of declared outputs to keep in sync.
Keys stamp input files by length and mtime rather than hashing contents - the same trade cargo and ninja make. A rebuild producing byte-identical output will miss, but keys stay cheap on gigabyte inputs. Key material drawn from the filesystem has to be sampled at runtime rather than at flow-build time, since the pipeline may be generated on a different machine than the one that runs it; the API takes an otherwise-unused runtime handle so that sampling early fails to compile.
Entries are installed atomically and stamped as a whole, so an interrupted run cannot leave a partial entry behind and an externally modified one is evicted rather than served forever. The store is size-capped and collected at the end of each local run, with cargo xflowey-cache for inspection and manual cleanup. Where no persistent dir is available - CI, typically - memoized steps degrade into plain steps, and FLOWEY_NO_MEMO=1 disables memoization entirely.
Converting the release-download node fixes a latent bug along the way. It previously downloaded straight to its final cache path, so an interrupted download left a truncated file that every later run treated as a permanent cache hit. Staging and atomic install make that unrepresentable.
Separately, direct_run_do_work moved the process working directory as it ran each step but never put it back, so its caller resolved relative paths against whatever directory the last step happened to leave behind. It now restores the directory on every exit path, which also fixes post-run cleanup that had been silently doing nothing.