fix(benchmark): the bench copy leaked because only root could delete it
The copy fix in a93a411 restored the checkout's single-writer invariant but
stranded the copy: 1.2 MB per run, growing forever.
`cargo bench` runs as root inside the container and writes `target/` there, so
the copy is root-owned. The server process is uid 65532; its
`remove_dir_all` cannot delete those files, and `Drop` discarded the error — so
the tree survived and nothing said so. The same "cleanup that cannot clean up"
shape as the container leak in the runtime tests, and invisible for the same
reason: a swallowed error on a path nobody reads.
`purge_copy` removes it from INSIDE the container, as root, where it was
written. Called on BOTH the success and failure paths before `Drop`, and again
before creating a copy, since a stale one from a previous run is root-owned too.
`Drop` stays as a fallback for the early-error paths where nothing ran as root
yet, and now says in its doc comment that it cannot do the real job.
Found by checking `_bench` after the uid probe went green — the invariant it
asserts was satisfied while the fix that satisfied it was leaking.
243 lib tests.
This commit is contained in:
@@ -171,11 +171,21 @@ pub async fn run(
|
|||||||
// same way: the harness's uid probe, reporting `uids=0,65532`. Measurement
|
// same way: the harness's uid probe, reporting `uids=0,65532`. Measurement
|
||||||
// must not mutate what it measures — the rule this codebase already applies
|
// must not mutate what it measures — the rule this codebase already applies
|
||||||
// to the judge and to the `verifier` subagent.
|
// to the judge and to the `verifier` subagent.
|
||||||
let copy = BenchCopy::of(&workdir, &bench_copy_path(mission_id))?;
|
let copy_root = bench_copy_path(mission_id);
|
||||||
|
// A stale copy from a previous run is ROOT-owned (see `purge_copy`), so it
|
||||||
|
// must be removed the same way it was created — from inside the container.
|
||||||
|
purge_copy(&container, ©_root).await;
|
||||||
|
let copy = BenchCopy::of(&workdir, ©_root)?;
|
||||||
let cmd = harness.command();
|
let cmd = harness.command();
|
||||||
let raw = docker_exec(&container, copy.workdir(), &cmd)
|
let result = docker_exec(&container, copy.workdir(), &cmd)
|
||||||
.await
|
.await
|
||||||
.map_err(|e| format!("exec {cmd:?}: {e}"))?;
|
.map_err(|e| format!("exec {cmd:?}: {e}"));
|
||||||
|
// Explicitly, on BOTH paths, before the `Drop` fallback runs. `cargo bench`
|
||||||
|
// writes `target/` as root, and the server process is uid 65532: its
|
||||||
|
// `remove_dir_all` cannot delete root-owned files and silently leaves the
|
||||||
|
// whole copy behind — measured at 1.2 MB per run, growing forever.
|
||||||
|
purge_copy(&container, ©_root).await;
|
||||||
|
let raw = result?;
|
||||||
let metrics = parse_output(&raw, &harness);
|
let metrics = parse_output(&raw, &harness);
|
||||||
Ok((metrics, harness.driver_name().to_string()))
|
Ok((metrics, harness.driver_name().to_string()))
|
||||||
}
|
}
|
||||||
@@ -257,6 +267,26 @@ fn bench_copy_path(mission_id: Uuid) -> std::path::PathBuf {
|
|||||||
.join(mission_id.to_string())
|
.join(mission_id.to_string())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Delete a benchmark copy from inside the container, as root.
|
||||||
|
///
|
||||||
|
/// The copy's `target/` belongs to root because the benchmark that created it
|
||||||
|
/// ran as root. `std::fs::remove_dir_all` from the server (uid 65532) fails on
|
||||||
|
/// those files, so the tree survives — quietly, because the error was
|
||||||
|
/// discarded. Removing it where it was written is the only thing that works.
|
||||||
|
async fn purge_copy(container: &str, root: &std::path::Path) {
|
||||||
|
let cmd = vec![
|
||||||
|
"rm".to_string(),
|
||||||
|
"-rf".to_string(),
|
||||||
|
root.display().to_string(),
|
||||||
|
];
|
||||||
|
if let Err(e) = docker_exec(container, std::path::Path::new("/"), &cmd).await {
|
||||||
|
eprintln!(
|
||||||
|
"benchmark_runner: could not remove bench copy {}: {e}",
|
||||||
|
root.display()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// A throwaway copy of the checkout for one benchmark run.
|
/// A throwaway copy of the checkout for one benchmark run.
|
||||||
///
|
///
|
||||||
/// Removed on drop, including on the error paths — a `target/` left behind is
|
/// Removed on drop, including on the error paths — a `target/` left behind is
|
||||||
@@ -289,6 +319,9 @@ impl BenchCopy {
|
|||||||
}
|
}
|
||||||
|
|
||||||
impl Drop for BenchCopy {
|
impl Drop for BenchCopy {
|
||||||
|
/// Best-effort only. This CANNOT remove the root-owned `target/` a benchmark
|
||||||
|
/// leaves behind — `purge_copy` is what actually clears it, and this stays
|
||||||
|
/// as a fallback for the early-error paths where nothing ran as root yet.
|
||||||
fn drop(&mut self) {
|
fn drop(&mut self) {
|
||||||
let _ = std::fs::remove_dir_all(&self.root);
|
let _ = std::fs::remove_dir_all(&self.root);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user