Skip to content

flowey: memoize expensive steps across local runs - #4055

Open
jstarks wants to merge 3 commits into
microsoft:mainfrom
jstarks:memo
Open

flowey: memoize expensive steps across local runs#4055
jstarks wants to merge 3 commits into
microsoft:mainfrom
jstarks:memo

Conversation

@jstarks

@jstarks jstarks commented Jul 28, 2026

Copy link
Copy Markdown
Member

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.

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.
@jstarks
jstarks requested a review from a team as a code owner July 28, 2026 15:49
Copilot AI review requested due to automatic review settings July 28, 2026 15:49
@jstarks
jstarks requested a review from a team as a code owner July 28, 2026 15:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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) and NodeCtx::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-cache command 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 uses unwrap() 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). Since in_bin comes 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_step but does fallible work (file_name().unwrap()) and would panic on an unexpected in_bin value. Consider making it a regular emit_rust_step so 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);
            }

Comment thread flowey/flowey_core/src/node.rs
Comment thread flowey/flowey_cli/src/pipeline_resolver/direct_run.rs Outdated
Comment thread flowey/flowey_core/src/node/memo.rs
Copilot AI review requested due to automatic review settings July 28, 2026 16:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 uses emit_minor_rust_stepv and create_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 after res?, 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 of direct_run_do_work success/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 if in_bin doesn'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 bad in_bin path will panic instead of surfacing as a step error. Consider making this a fallible step and returning a clear error if in_bin doesn'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.json is unreadable, entries() currently reports size_bytes = 0. That undercounts stats() and can prevent gc() from evicting oversized stores (since eviction uses the summed size_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() unwraps MemoStore::open(...), which will panic if the store can't be created/opened (e.g. permissions, bad path). This should return an anyhow::Result via emit_rust_stepv so 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()
            }),

@github-actions

Copy link
Copy Markdown

Copilot AI review requested due to automatic review settings July 28, 2026 20:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 always Some and uses unwrap(). If in_bin ever 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 paths is emitted as a minor step (cannot fail) but uses file_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 uses emit_minor_rust_stepv and unwrap() for create_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 propagates anyhow::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 uses emit_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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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| {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Didn't you tell me not to do exactly this in a past pr lol

Comment on lines +111 to +117
/// 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yay, cache coherency! Everyone's favorite source of heisenbugs lol

@smalis-msft smalis-msft left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While this is neat, how much time does it tend to save? It's a significant amount of extra complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants