refactor: the judge's sandbox joins the other three copy sites on root_copy
Four places copy a mission checkout so a ROOT command can run against it without touching the live tree: the judge, the benchmark runner, the on_green_tests gate, and — until now — the judge again, with its own implementation predating the shared one. `evaluator_tools::Sandbox::for_checkout` now builds through `root_copy::RootCopy`. Same packer, same exclusion list, same reasoning in one place. It needs the copy to OUTLIVE the handle, because the judge has not run when `for_checkout` returns and a firing `Drop` would delete the tree out from under it. That is `into_workdir`, a method rather than a `mem::forget` at the call site: the transfer of cleanup responsibility is then visible in the type instead of implied by a leak. `Sandbox::purge` remains what actually clears it, since only a container running as root can remove the root-owned `target/`. What did NOT move: `pack_dir` in the microVM inject/collect path. That marshals a tree to and from a guest over vsock — a transport, not a host-side copy — and folding it in would merge two things that only look alike. 249 lib tests.
This commit is contained in:
@@ -255,19 +255,21 @@ impl Sandbox {
|
|||||||
if !source.is_dir() {
|
if !source.is_dir() {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
// A stale copy from a previous pass would be verified instead of this
|
// `root_copy` owns this pattern for all four callers — the judge, the
|
||||||
// one's work — the same "judged a tree nobody wrote" shape the evaluator
|
// benchmark runner, the on_green_tests gate, and this. It packs through
|
||||||
// exists to prevent.
|
// the transport packer (one exclusion list, so a copy carries exactly
|
||||||
let _ = std::fs::remove_dir_all(root);
|
// what a delivered diff carries) and its `purge` is the only thing that
|
||||||
// Through the transport packer, so the copy carries exactly what the
|
// can remove the root-owned `target/` a run leaves behind.
|
||||||
// delivered diff carries: no `target/`, no `node_modules/`. One
|
//
|
||||||
// exclusion list, three consumers.
|
// A stale copy would otherwise be verified instead of this pass's work —
|
||||||
let archive = crate::mission_fs::pack_dir(source, "repo").ok()?;
|
// the "judged a tree nobody wrote" shape the evaluator exists to prevent
|
||||||
crate::mission_fs::unpack_into(&archive, root).ok()?;
|
// — so the caller purges before constructing.
|
||||||
let workdir = root.join("repo");
|
// `into_workdir` because the judge has not run yet: letting the handle's
|
||||||
if !workdir.is_dir() {
|
// Drop fire on return would delete the tree out from under it. `Sandbox`
|
||||||
return None;
|
// owns the lifetime from here, and `Sandbox::purge` clears it.
|
||||||
}
|
let workdir = crate::root_copy::RootCopy::of(source, root)
|
||||||
|
.ok()?
|
||||||
|
.into_workdir();
|
||||||
let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER")
|
let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER")
|
||||||
.unwrap_or_else(|_| "clawmates-runtime".to_string());
|
.unwrap_or_else(|_| "clawmates-runtime".to_string());
|
||||||
Some(Sandbox {
|
Some(Sandbox {
|
||||||
|
|||||||
@@ -4,7 +4,8 @@
|
|||||||
//! the judge's verification (`evaluator_tools::Sandbox`), the benchmark runner,
|
//! the judge's verification (`evaluator_tools::Sandbox`), the benchmark runner,
|
||||||
//! and the `on_green_tests` delivery gate. All three enter a container running as
|
//! and the `on_green_tests` delivery gate. All three enter a container running as
|
||||||
//! root with the missions root bind-mounted, and all three run something that
|
//! root with the missions root bind-mounted, and all three run something that
|
||||||
//! writes `target/`.
|
//! writes `target/`. All three now go through here; the judge was the last to
|
||||||
|
//! move, having carried its own copy of this logic since before it existed.
|
||||||
//!
|
//!
|
||||||
//! Run against the live checkout, that breaks the single-writer invariant: the
|
//! Run against the live checkout, that breaks the single-writer invariant: the
|
||||||
//! tree is owned by uid 65532 and now contains root-owned build output, so the
|
//! tree is owned by uid 65532 and now contains root-owned build output, so the
|
||||||
@@ -94,6 +95,23 @@ impl RootCopy {
|
|||||||
pub fn workdir(&self) -> &Path {
|
pub fn workdir(&self) -> &Path {
|
||||||
&self.workdir
|
&self.workdir
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Take the working directory and give up automatic cleanup.
|
||||||
|
///
|
||||||
|
/// For a caller whose copy outlives this handle — `evaluator_tools::Sandbox`
|
||||||
|
/// hands the path to a judge that has not run yet, so letting `Drop` fire on
|
||||||
|
/// return would delete the tree out from under it. That caller becomes
|
||||||
|
/// responsible for calling [`purge`], which is the only thing that can
|
||||||
|
/// remove root-owned build output anyway.
|
||||||
|
///
|
||||||
|
/// Spelled as a method rather than `mem::forget` at the call site, so the
|
||||||
|
/// transfer of responsibility is visible in the type rather than implied by
|
||||||
|
/// a leak.
|
||||||
|
pub fn into_workdir(self) -> PathBuf {
|
||||||
|
let workdir = self.workdir.clone();
|
||||||
|
std::mem::forget(self);
|
||||||
|
workdir
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Drop for RootCopy {
|
impl Drop for RootCopy {
|
||||||
@@ -115,7 +133,7 @@ mod tests {
|
|||||||
fn copies_live_beside_the_mission_directory_not_inside_it() {
|
fn copies_live_beside_the_mission_directory_not_inside_it() {
|
||||||
let mission = uuid::Uuid::now_v7();
|
let mission = uuid::Uuid::now_v7();
|
||||||
let mission_dir = crate::mission_workspace::missions_root().join(mission.to_string());
|
let mission_dir = crate::mission_workspace::missions_root().join(mission.to_string());
|
||||||
for kind in ["_bench", "_gate"] {
|
for kind in ["_bench", "_gate", "_verify"] {
|
||||||
let root = copy_root(kind, mission);
|
let root = copy_root(kind, mission);
|
||||||
assert!(!root.starts_with(&mission_dir), "{root:?}");
|
assert!(!root.starts_with(&mission_dir), "{root:?}");
|
||||||
assert!(
|
assert!(
|
||||||
@@ -131,7 +149,8 @@ mod tests {
|
|||||||
fn a_copy_is_never_the_checkout() {
|
fn a_copy_is_never_the_checkout() {
|
||||||
let mission = uuid::Uuid::now_v7();
|
let mission = uuid::Uuid::now_v7();
|
||||||
let live = crate::mission_workspace::checkout_path(mission);
|
let live = crate::mission_workspace::checkout_path(mission);
|
||||||
assert_ne!(copy_root("_bench", mission).join("repo"), live);
|
for kind in ["_bench", "_gate", "_verify"] {
|
||||||
assert_ne!(copy_root("_gate", mission).join("repo"), live);
|
assert_ne!(copy_root(kind, mission).join("repo"), live);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user