fix(evaluator): the verification sandbox leaked for the same reason the bench copy did
Found by checking `_verify` after fixing the identical bug in `_bench`: 16 MB
stranded across two copies, the oldest hours old.
`Sandbox::Drop` calls `std::fs::remove_dir_all` as uid 65532. The judge runs
`cargo test` in a container as ROOT — that is the entire point of the sandbox —
so the copy's `target/` is root-owned and the removal fails on it, leaving the
whole tree. The error was logged to a stream nobody reads, so the sandbox that
exists to protect the checkout quietly filled the disk instead.
Its doc comment also claimed "the copy lives under `_verify/<mission>`, which
the next pass clears anyway". That was wrong for exactly the same reason:
`for_checkout` removes a stale root before copying, with the same uid, and fails
the same way. A leaked copy was permanent, not transient.
`Sandbox::purge` removes it from inside the container, as root, where it was
written. `evaluate` now wraps its body so the purge runs on EVERY exit — that
function returns from several branches, and cleanup only some paths reach is the
same as no cleanup on the others. `Drop` stays as a fallback for the early paths
where nothing has run as root yet, and its comment no longer claims otherwise.
This is the third instance today of the same shape: cleanup that cannot clean up,
invisible because the failure was swallowed. The others were the leaked agent
containers in the runtime tests and the bench copy in e89a32f.
243 lib tests.
This commit is contained in:
@@ -424,6 +424,12 @@ pub async fn evaluate(
|
|||||||
"COMPLETION CONDITION:\n{condition}\n\nEVIDENCE (agent claims — verify them):\n{evidence}"
|
"COMPLETION CONDITION:\n{condition}\n\nEVIDENCE (agent claims — verify them):\n{evidence}"
|
||||||
);
|
);
|
||||||
let sandbox = crate::evaluator_tools::Sandbox::for_mission(mission_id);
|
let sandbox = crate::evaluator_tools::Sandbox::for_mission(mission_id);
|
||||||
|
// Purged explicitly at every exit below: `Drop` runs as uid 65532 and cannot
|
||||||
|
// delete the root-owned `target/` the judge's own `cargo test` leaves behind.
|
||||||
|
// Wrapped so the purge below runs on EVERY exit: this function returns
|
||||||
|
// from several branches, and a cleanup only some paths reach is the same
|
||||||
|
// as no cleanup on the others.
|
||||||
|
let verdict = async {
|
||||||
|
|
||||||
// Most preferred: a judge from a DIFFERENT provider family, with the same
|
// Most preferred: a judge from a DIFFERENT provider family, with the same
|
||||||
// allow-listed tool loop. Claude judging Claude's work is a correlated
|
// allow-listed tool loop. Claude judging Claude's work is a correlated
|
||||||
@@ -522,6 +528,12 @@ pub async fn evaluate(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
.await;
|
||||||
|
if let Some(sb) = &sandbox {
|
||||||
|
sb.purge().await;
|
||||||
|
}
|
||||||
|
verdict
|
||||||
|
}
|
||||||
|
|
||||||
/// Ceiling on verification commands per verdict. A judge that has run twelve
|
/// Ceiling on verification commands per verdict. A judge that has run twelve
|
||||||
/// commands and still cannot tell is not going to be rescued by a thirteenth,
|
/// commands and still cannot tell is not going to be rescued by a thirteenth,
|
||||||
|
|||||||
@@ -392,12 +392,57 @@ fn git_ownership_env(workdir: &str) -> Vec<String> {
|
|||||||
]
|
]
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Drop for Sandbox {
|
impl Sandbox {
|
||||||
/// Remove the copy when the evaluation is done.
|
/// Remove the copy, from inside the container that wrote it.
|
||||||
///
|
///
|
||||||
/// Best-effort: a leaked copy costs disk, and failing an evaluation over
|
/// `Drop` cannot do this. The judge runs `cargo test` in a container as
|
||||||
/// cleanup would trade a real verdict for a housekeeping error. The copy
|
/// ROOT, so the copy's `target/` is root-owned, and the server process is
|
||||||
/// lives under `_verify/<mission>`, which the next pass clears anyway.
|
/// uid 65532 — its `remove_dir_all` fails on those files and leaves the
|
||||||
|
/// whole tree behind. Measured: 16 MB across two stranded copies, the oldest
|
||||||
|
/// hours old, while `Drop` logged nothing anyone read.
|
||||||
|
///
|
||||||
|
/// The claim that "the next pass clears anyway" was wrong for the same
|
||||||
|
/// reason: `for_checkout` removes a stale root before copying, with the same
|
||||||
|
/// uid, and fails the same way.
|
||||||
|
///
|
||||||
|
/// Still best-effort — a housekeeping error must not cost a real verdict —
|
||||||
|
/// but now attempted by something that can actually succeed.
|
||||||
|
pub async fn purge(&self) {
|
||||||
|
if !self.owned {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
let Some(root) = self.workdir.parent() else {
|
||||||
|
return;
|
||||||
|
};
|
||||||
|
let Ok(docker) = crate::container_exec::connect() else {
|
||||||
|
return;
|
||||||
|
};
|
||||||
|
let argv = vec![
|
||||||
|
"rm".to_string(),
|
||||||
|
"-rf".to_string(),
|
||||||
|
root.display().to_string(),
|
||||||
|
];
|
||||||
|
if let Err(e) = crate::container_exec::exec(
|
||||||
|
&docker,
|
||||||
|
&self.container,
|
||||||
|
Some("/"),
|
||||||
|
&argv,
|
||||||
|
COMMAND_TIMEOUT,
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
eprintln!(
|
||||||
|
"evaluator_tools: could not remove the verification copy at {} ({e})",
|
||||||
|
root.display()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
impl Drop for Sandbox {
|
||||||
|
/// Fallback only — see [`Sandbox::purge`], which is what actually clears a
|
||||||
|
/// copy the judge has run commands in. This still catches the early paths
|
||||||
|
/// where nothing has run as root yet.
|
||||||
fn drop(&mut self) {
|
fn drop(&mut self) {
|
||||||
if !self.owned {
|
if !self.owned {
|
||||||
return;
|
return;
|
||||||
|
|||||||
Reference in New Issue
Block a user