Reuse staged Linux snapshots by content hash#571
Merged
justinmoon merged 3 commits intomasterfrom Mar 10, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe changes implement content-addressed snapshot and runner flake reuse by computing SHA-256 content hashes, storing them in JSON metadata files, and modifying executor and run logic to match local and remote hashes for selective reuse, failing on mismatches. Changes
Sequence DiagramsequenceDiagram
participant Local as Local Build
participant Executor as Remote Executor
participant Remote as Remote Host
participant Metadata as Remote Metadata File
Local->>Local: Compute snapshot content_hash
Local->>Executor: Initiate remote execution
Executor->>Remote: Check if remote snapshot dir exists
Remote-->>Executor: Path exists/not exists
alt Snapshot exists
Executor->>Metadata: Read remote pikaci-snapshot.json
Metadata-->>Executor: Return remote content_hash
Executor->>Executor: Compare hashes
alt Hashes match
Executor-->>Local: Reuse existing remote snapshot
else Hashes mismatch
Executor-->>Local: Fail with metadata mismatch error
end
else Snapshot missing
Local->>Remote: Sync snapshot to remote
Local->>Metadata: Write content_hash to remote metadata
Executor-->>Local: Snapshot synced and ready
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/pikaci/src/executor.rs (2)
1319-1374:⚠️ Potential issue | 🔴 CriticalPublish hash-addressed snapshots atomically.
Line 1321 treats
pikaci-snapshot.jsonin the finalsnapshots/<hash>/snapshotdirectory as the reuse signal, but Lines 1367-1373 populate that same shared directory in place. Two matching runs can now race: one can observe the metadata before extraction finishes, orrm -rfthe shared directory while another run is already using it. Please stage into a temp dir and atomically rename/create the ready marker only after the sync completes, or guard per-hash publication with a remote lock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pikaci/src/executor.rs` around lines 1319 - 1374, The code is using the presence of ready_marker (pikaci-snapshot.json in remote_snapshot_dir) as the reuse signal while also populating that same remote_snapshot_dir in-place, causing race conditions; change the publish flow so sync_directory_to_remote writes the snapshot into a temp/staging directory on the remote (e.g. remote_snapshot_dir.tmp or use a unique temp path), complete all extraction/sync there, and only after successful sync atomically create/rename the ready marker or rename the staged directory into remote_snapshot_dir (or acquire a per-hash remote lock) so that load_remote_snapshot_metadata/read_snapshot_metadata see the ready marker only after the snapshot is fully published; update calls around ready_marker, read_snapshot_metadata, load_remote_snapshot_metadata, append_line and sync_directory_to_remote accordingly to use the staging path and atomic rename/marker creation.
1661-1727:⚠️ Potential issue | 🔴 CriticalSerialize writes to
runner-flakes/<hash>.
remote_flake_rootis now shared across runs by content hash, but on a miss we still sync into the final directory with the same destructiverm -rfflow. Two runs building the same flake hash can delete the flake out from under each other between sync, build, and metadata write, causing nondeterministic remote builds. This cache needs the same temp-dir + atomic publish or per-hash locking strategy as the snapshot cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pikaci/src/executor.rs` around lines 1661 - 1727, The remote "runner-flakes/<hash>" path (remote_flake_root / flake_hash) can be clobbered by concurrent runs; change the publish flow in the remote flake path handling (symbols: remote_flake_root, remote_flake_dir, sync_directory_to_remote, remote_symlink, load_remote_runner_flake_metadata, remote_store_path) to serialize writes by using a per-hash atomic publish or lock: on miss, create a unique temp directory under remote_work_dir (e.g. runner-flakes/.tmp-<uuid>), sync into that temp with sync_directory_to_remote, perform any build/verify steps, write the metadata into the temp, then atomically rename/move the temp to the final directory (or acquire a per-hash lock on flake_hash before syncing and release after metadata write); also re-check load_remote_runner_flake_metadata after acquiring the lock/after rename to avoid racing with concurrent publishers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pikaci/src/executor.rs`:
- Around line 1221-1229: remote_artifacts_dir is currently set to
shared_job_dir.join("artifacts") which places artifacts under
`${remote_work_dir}/jobs/${job.id}` and can collide between runs; change it to
be scoped to the run by using remote_job_dir.join("artifacts") (or include
run_id in the path) when constructing the RemoteMicrovmContext so artifacts live
under the same `${remote_work_dir}/runs/${run_id}/...` as the rest of the state;
update any related logic that assumes the old path (e.g.,
reset_remote_microvm_artifacts) to reference
RemoteMicrovmContext.remote_artifacts_dir accordingly.
In `@crates/pikaci/src/run.rs`:
- Around line 3104-3118: staged_linux_remote_snapshot_dir is being called with a
potentially nested installable path which causes it to read pikaci-snapshot.json
from the wrong location; instead, call staged_linux_remote_snapshot_dir with the
actual snapshot root (i.e., the non-nested "snapshot" root) to compute
remote_snapshot_root, then compute remote_snapshot_dir by appending the nested
suffix (using snapshot_relative.strip_prefix("snapshot") -> nested ->
remote_snapshot_root.join(nested)); also ensure the later sync that uses
remote_snapshot_root (lines ~3139-3144) still syncs the root so the copied tree
matches remote_installable.
In `@crates/pikaci/src/snapshot.rs`:
- Around line 222-251: The snapshot hash currently omits file permission bits
and does not length-prefix file contents; update hash_snapshot_tree so that for
files (metadata.is_file()) you include the file's mode/permission bits (from
metadata.permissions()/platform-specific mode) and the file length
(metadata.len()) into the hasher before streaming content, and for symlinks
include any relevant permission/mode info from the symlink's metadata as well;
keep using the existing hasher.update(...) calls and the existing relative/path
markers and separators so you only add the mode and length values (as
deterministic byte sequences) to prevent chmod-only or delimiter-collision
collisions.
In `@todos/pikaci-staged-ci-plan.md`:
- Line 910: The remote artifacts directory is currently job-scoped and reset
destructively by reset_remote_microvm_artifacts(), enabling concurrent runs to
clobber each other; change the artifacts root to be run-scoped by including the
run identifier (e.g., run.id or run.uuid) in the path used when
mounting/creating artifacts under
'/var/tmp/pikaci-prepared-output/jobs/<job.id>/artifacts' (or alternatively
remove the destructive reset in reset_remote_microvm_artifacts() and make that
function operate on a run-specific path), and ensure any code that references
the runner flake cache still uses its content-addressed path while writable
artifacts use the new per-run path.
---
Outside diff comments:
In `@crates/pikaci/src/executor.rs`:
- Around line 1319-1374: The code is using the presence of ready_marker
(pikaci-snapshot.json in remote_snapshot_dir) as the reuse signal while also
populating that same remote_snapshot_dir in-place, causing race conditions;
change the publish flow so sync_directory_to_remote writes the snapshot into a
temp/staging directory on the remote (e.g. remote_snapshot_dir.tmp or use a
unique temp path), complete all extraction/sync there, and only after successful
sync atomically create/rename the ready marker or rename the staged directory
into remote_snapshot_dir (or acquire a per-hash remote lock) so that
load_remote_snapshot_metadata/read_snapshot_metadata see the ready marker only
after the snapshot is fully published; update calls around ready_marker,
read_snapshot_metadata, load_remote_snapshot_metadata, append_line and
sync_directory_to_remote accordingly to use the staging path and atomic
rename/marker creation.
- Around line 1661-1727: The remote "runner-flakes/<hash>" path
(remote_flake_root / flake_hash) can be clobbered by concurrent runs; change the
publish flow in the remote flake path handling (symbols: remote_flake_root,
remote_flake_dir, sync_directory_to_remote, remote_symlink,
load_remote_runner_flake_metadata, remote_store_path) to serialize writes by
using a per-hash atomic publish or lock: on miss, create a unique temp directory
under remote_work_dir (e.g. runner-flakes/.tmp-<uuid>), sync into that temp with
sync_directory_to_remote, perform any build/verify steps, write the metadata
into the temp, then atomically rename/move the temp to the final directory (or
acquire a per-hash lock on flake_hash before syncing and release after metadata
write); also re-check load_remote_runner_flake_metadata after acquiring the
lock/after rename to avoid racing with concurrent publishers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2147cd98-4600-4936-8257-57837fe54c63
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/pikaci/Cargo.tomlcrates/pikaci/src/executor.rscrates/pikaci/src/run.rscrates/pikaci/src/snapshot.rstodos/pikaci-staged-ci-plan.md
Comment on lines
1221
to
1229
| let remote_job_dir = remote_run_dir.join("jobs").join(job.id); | ||
| let shared_job_dir = remote_work_dir.join("jobs").join(job.id); | ||
| Ok(RemoteMicrovmContext { | ||
| remote_host, | ||
| remote_snapshot_dir: remote_run_dir.join("snapshot"), | ||
| remote_work_dir: remote_work_dir.clone(), | ||
| remote_snapshot_dir, | ||
| remote_vm_dir: remote_job_dir.join("vm"), | ||
| remote_artifacts_dir: remote_job_dir.join("artifacts"), | ||
| remote_artifacts_dir: shared_job_dir.join("artifacts"), | ||
| remote_cargo_home_dir: remote_work_dir.join("cache").join("cargo-home"), |
There was a problem hiding this comment.
Keep remote artifacts scoped to the run.
remote_artifacts_dir now lives under ${remote_work_dir}/jobs/${job.id}, while the rest of the execution state stays under ${remote_work_dir}/runs/${run_id}/.... Combined with reset_remote_microvm_artifacts, two runs of the same job id will delete or overwrite each other’s guest.log and result.json. This needs to stay under remote_job_dir or include run_id in the shared path.
Suggested fix
- remote_artifacts_dir: shared_job_dir.join("artifacts"),
+ remote_artifacts_dir: remote_job_dir.join("artifacts"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let remote_job_dir = remote_run_dir.join("jobs").join(job.id); | |
| let shared_job_dir = remote_work_dir.join("jobs").join(job.id); | |
| Ok(RemoteMicrovmContext { | |
| remote_host, | |
| remote_snapshot_dir: remote_run_dir.join("snapshot"), | |
| remote_work_dir: remote_work_dir.clone(), | |
| remote_snapshot_dir, | |
| remote_vm_dir: remote_job_dir.join("vm"), | |
| remote_artifacts_dir: remote_job_dir.join("artifacts"), | |
| remote_artifacts_dir: shared_job_dir.join("artifacts"), | |
| remote_cargo_home_dir: remote_work_dir.join("cache").join("cargo-home"), | |
| let remote_job_dir = remote_run_dir.join("jobs").join(job.id); | |
| let shared_job_dir = remote_work_dir.join("jobs").join(job.id); | |
| Ok(RemoteMicrovmContext { | |
| remote_host, | |
| remote_work_dir: remote_work_dir.clone(), | |
| remote_snapshot_dir, | |
| remote_vm_dir: remote_job_dir.join("vm"), | |
| remote_artifacts_dir: remote_job_dir.join("artifacts"), | |
| remote_cargo_home_dir: remote_work_dir.join("cache").join("cargo-home"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pikaci/src/executor.rs` around lines 1221 - 1229, remote_artifacts_dir
is currently set to shared_job_dir.join("artifacts") which places artifacts
under `${remote_work_dir}/jobs/${job.id}` and can collide between runs; change
it to be scoped to the run by using remote_job_dir.join("artifacts") (or include
run_id in the path) when constructing the RemoteMicrovmContext so artifacts live
under the same `${remote_work_dir}/runs/${run_id}/...` as the rest of the state;
update any related logic that assumes the old path (e.g.,
reset_remote_microvm_artifacts) to reference
RemoteMicrovmContext.remote_artifacts_dir accordingly.
Comment on lines
+3104
to
+3118
| let remote_snapshot_root = | ||
| staged_linux_remote_snapshot_dir(&snapshot_dir, remote_work_dir, run_id)?; | ||
| let remote_snapshot_dir = if snapshot_relative == Path::new("snapshot") { | ||
| remote_snapshot_root | ||
| } else { | ||
| let nested = snapshot_relative | ||
| .strip_prefix("snapshot") | ||
| .with_context(|| { | ||
| format!( | ||
| "translate nested staged Linux Rust snapshot path {}", | ||
| snapshot_relative.display() | ||
| ) | ||
| })?; | ||
| remote_snapshot_root.join(nested) | ||
| }; |
There was a problem hiding this comment.
Keep the snapshot root separate from the translated installable path.
Line 3105 passes the possibly nested installable directory into staged_linux_remote_snapshot_dir(). That helper reads pikaci-snapshot.json from the exact path it is given (crates/pikaci/src/executor.rs, Lines 1240-1253), so .../snapshot/<subdir>#... will miss the stored content_hash and fall back to /runs/<run_id>/snapshot. Lines 3139-3144 still sync the snapshot root, which means the copied remote tree no longer matches remote_installable when the nested branch is exercised. Derive remote_snapshot_root from the actual snapshot root and append the nested suffix afterwards.
💡 Suggested fix
+ let local_snapshot_root = run_dir.join("snapshot");
let run_id = run_dir
.file_name()
.and_then(|value| value.to_str())
.ok_or_else(|| anyhow!("derive run id from {}", run_dir.display()))?;
let remote_snapshot_root =
- staged_linux_remote_snapshot_dir(&snapshot_dir, remote_work_dir, run_id)?;
+ staged_linux_remote_snapshot_dir(&local_snapshot_root, remote_work_dir, run_id)?;Also applies to: 3139-3144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pikaci/src/run.rs` around lines 3104 - 3118,
staged_linux_remote_snapshot_dir is being called with a potentially nested
installable path which causes it to read pikaci-snapshot.json from the wrong
location; instead, call staged_linux_remote_snapshot_dir with the actual
snapshot root (i.e., the non-nested "snapshot" root) to compute
remote_snapshot_root, then compute remote_snapshot_dir by appending the nested
suffix (using snapshot_relative.strip_prefix("snapshot") -> nested ->
remote_snapshot_root.join(nested)); also ensure the later sync that uses
remote_snapshot_root (lines ~3139-3144) still syncs the root so the copied tree
matches remote_installable.
Comment on lines
+222
to
+251
| if metadata.file_type().is_symlink() { | ||
| hasher.update(b"symlink\0"); | ||
| hasher.update(relative.as_os_str().as_encoded_bytes()); | ||
| hasher.update(b"\0"); | ||
| let target = | ||
| fs::read_link(&path).with_context(|| format!("read symlink {}", path.display()))?; | ||
| hasher.update(target.as_os_str().as_encoded_bytes()); | ||
| hasher.update(b"\0"); | ||
| } else if metadata.is_dir() { | ||
| hasher.update(b"dir\0"); | ||
| hasher.update(relative.as_os_str().as_encoded_bytes()); | ||
| hasher.update(b"\0"); | ||
| hash_snapshot_tree(root, &path, hasher)?; | ||
| } else if metadata.is_file() { | ||
| hasher.update(b"file\0"); | ||
| hasher.update(relative.as_os_str().as_encoded_bytes()); | ||
| hasher.update(b"\0"); | ||
| let mut file = | ||
| fs::File::open(&path).with_context(|| format!("open {}", path.display()))?; | ||
| let mut buffer = [0u8; 8192]; | ||
| loop { | ||
| let read = file | ||
| .read(&mut buffer) | ||
| .with_context(|| format!("read {}", path.display()))?; | ||
| if read == 0 { | ||
| break; | ||
| } | ||
| hasher.update(&buffer[..read]); | ||
| } | ||
| hasher.update(b"\0"); |
There was a problem hiding this comment.
Hash file mode and content length, not just raw bytes.
Lines 236-251 ignore permission bits and do not length-prefix file contents. A chmod-only change therefore keeps the same snapshot hash, and a binary file containing delimiter bytes can serialize to the same digest stream as multiple later entries. Since this hash now controls content-addressed snapshot reuse, two distinct trees can alias the same remote snapshot.
💡 Suggested fix
-use std::os::unix::fs as unix_fs;
+use std::os::unix::fs::{self as unix_fs, PermissionsExt};
...
} else if metadata.is_file() {
hasher.update(b"file\0");
hasher.update(relative.as_os_str().as_encoded_bytes());
hasher.update(b"\0");
+ hasher.update(metadata.permissions().mode().to_be_bytes());
+ hasher.update(metadata.len().to_be_bytes());
let mut file =
fs::File::open(&path).with_context(|| format!("open {}", path.display()))?;
let mut buffer = [0u8; 8192];
loop {
let read = file
.read(&mut buffer)
.with_context(|| format!("read {}", path.display()))?;
if read == 0 {
break;
}
hasher.update(&buffer[..read]);
}
- hasher.update(b"\0");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pikaci/src/snapshot.rs` around lines 222 - 251, The snapshot hash
currently omits file permission bits and does not length-prefix file contents;
update hash_snapshot_tree so that for files (metadata.is_file()) you include the
file's mode/permission bits (from metadata.permissions()/platform-specific mode)
and the file length (metadata.len()) into the hasher before streaming content,
and for symlinks include any relevant permission/mode info from the symlink's
metadata as well; keep using the existing hasher.update(...) calls and the
existing relative/path markers and separators so you only add the mode and
length values (as deterministic byte sequences) to prevent chmod-only or
delimiter-collision collisions.
| - note that the next reuse slice moved that remaining per-run runner staging onto the same strict content-addressed footing: | ||
| - each staged Linux remote microVM runner flake is now keyed by the SHA-256 content hash of its rendered `flake.nix`, under `/var/tmp/pikaci-prepared-output/runner-flakes/<hash>/flake` on `pika-build`, | ||
| - `pikaci` records `pikaci-runner-flake.json` next to that remote flake payload with the exact `content_hash` and realized remote runner store path, then validates both on reuse by requiring an exact hash match and a still-existing remote store path; it now hard-fails on mismatch or stale metadata instead of silently re-rendering ambiguous state, | ||
| - the remote artifacts mount for staged Linux remote microVM jobs now points at the shared per-job path under `/var/tmp/pikaci-prepared-output/jobs/<job>/artifacts`, and each run resets that directory before launch so the rendered runner flake stays content-stable across unchanged reruns, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how remote artifact directories are derived.
rg -n -C4 'prepared-output/jobs|jobs/.*/artifacts|artifacts_dir|remote.*artifacts' \
crates/pikaci/src/executor.rs crates/pikaci/src/run.rs
# Inspect whether launch/reset code removes or recreates those directories.
rg -n -C4 'remove_dir_all|create_dir_all|reset.*artifacts|artifacts.*reset' \
crates/pikaci/src/executor.rs crates/pikaci/src/run.rsRepository: sledtools/pika
Length of output: 50370
🏁 Script executed:
rg -n 'shared_job_dir' crates/pikaci/src/executor.rs | head -20Repository: sledtools/pika
Length of output: 310
🏁 Script executed:
rg -n 'remote_work_dir.*=' crates/pikaci/src/executor.rs | grep -v '//' | head -20Repository: sledtools/pika
Length of output: 103
🏁 Script executed:
sed -n '1200,1240p' crates/pikaci/src/executor.rsRepository: sledtools/pika
Length of output: 1888
Make the remote artifacts dir run-scoped, not just job-scoped.
The remote artifacts path is /var/tmp/pikaci-prepared-output/jobs/<job.id>/artifacts (keyed only by job ID), and reset_remote_microvm_artifacts() destructively removes and recreates it before every launch. This allows overlapping runs of the same lane to clobber each other's artifacts on pika-build. Keep the runner flake cache content-addressed, but isolate the writable artifacts root per run—either by including the run identifier in the path or by removing the destructive reset of the shared mutable location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@todos/pikaci-staged-ci-plan.md` at line 910, The remote artifacts directory
is currently job-scoped and reset destructively by
reset_remote_microvm_artifacts(), enabling concurrent runs to clobber each
other; change the artifacts root to be run-scoped by including the run
identifier (e.g., run.id or run.uuid) in the path used when mounting/creating
artifacts under '/var/tmp/pikaci-prepared-output/jobs/<job.id>/artifacts' (or
alternatively remove the destructive reset in reset_remote_microvm_artifacts()
and make that function operate on a run-specific path), and ensure any code that
references the runner flake cache still uses its content-addressed path while
writable artifacts use the new per-run path.
justinmoon
added a commit
that referenced
this pull request
Mar 11, 2026
* Reuse staged Linux snapshots by content hash * Record pikaci snapshot hash dependencies * Reuse staged Linux runner flakes remotely
This was referenced Mar 15, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements