fix(missions): an empty repo-less phase was re-processed on every tick forever
The guard added in ceab28b fails a repo-less phase that produced nothing. It
does not record that it looked — and the selection query asks "no artifact of
this kind exists", which stays true forever for a phase with no output. So the
phase matched on every sweep: a docker copy_out per tick, and with BATCH = 5,
five such phases would occupy every slot permanently and no repo-less mission
would ever be captured again.
Measured on the first live negative control: 4 occurrences of the guard's log
line, then 8 45 seconds later.
This is a bug this codebase has already fixed once. `record_uncapturable` exists
because "five reaped phases from earlier runs blocked the batch while a freshly
finished coding phase went untouched" — its own comment. I wrote the same defect
into new code on the same sweep, which is the argument for the marker being part
of the pattern rather than something each capture path remembers separately.
Same fix as the precedent: a real file (`NO-OUTPUT.md`) behind a real artifact
row, because a row pointing at nothing turns every reader into an unexplained
404. It carries `metadata.empty = true`, the convention `mission_delivery`
already uses for its "No code changes" artifact, so "captured, and there was
nothing" is distinguishable from "captured eight documents".
The guard itself was proven correct on that same run before this was noticed:
mission failed, phase failed, artifacts 0, with the reason and the
`allow_empty` escape hatch named in the log.
237 lib tests pass.
This commit is contained in:
@@ -60,6 +60,9 @@ const BATCH: i64 = 5;
|
||||
/// one of these has already been captured.
|
||||
pub const OUTPUT_KIND: &str = "document";
|
||||
|
||||
/// Filename of the marker written when a phase produced nothing.
|
||||
const EMPTY_MARKER: &str = "NO-OUTPUT.md";
|
||||
|
||||
/// Capture the outputs of finished phases on missions that have no repo.
|
||||
pub async fn capture_repo_less_phases(pool: &PgPool) -> Result<(), String> {
|
||||
let rows = sqlx::query(
|
||||
@@ -143,6 +146,25 @@ pub async fn capture_repo_less_phases(pool: &PgPool) -> Result<(), String> {
|
||||
}
|
||||
}
|
||||
|
||||
if captured.is_empty() {
|
||||
// Register a marker even when there is nothing to capture, or this
|
||||
// phase matches the `NOT EXISTS` selection on every tick forever:
|
||||
// re-running a docker copy_out each time and, because the batch is
|
||||
// bounded, permanently occupying a slot so no other repo-less
|
||||
// mission is ever captured again.
|
||||
//
|
||||
// `phase_runner::record_uncapturable` exists for exactly this
|
||||
// failure on the diff path — five dead phases starved the batch
|
||||
// while live work went untouched — and this code hit it again on
|
||||
// its first live negative control (4 log lines, then 8, 45 seconds
|
||||
// apart). Same shape, same fix: a real file behind a real row,
|
||||
// because an artifact pointing at nothing turns every reader into
|
||||
// an unexplained 404.
|
||||
if let Err(e) = register_empty_marker(pool, mission_id, phase_id, &dest).await {
|
||||
eprintln!("mission_outputs: marking phase {phase_id} as empty: {e}");
|
||||
}
|
||||
}
|
||||
|
||||
if captured.is_empty() && !allow_empty(&config) {
|
||||
// The same rule `empty_delivery_is_a_failure` applies to a coding
|
||||
// phase, for the only channel a repo-less phase has. Without it a
|
||||
@@ -171,6 +193,49 @@ pub async fn capture_repo_less_phases(pool: &PgPool) -> Result<(), String> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Record that a phase produced nothing, so it is not reconsidered forever.
|
||||
///
|
||||
/// Deliberately the same `OUTPUT_KIND` the real captures use: the selection
|
||||
/// query asks "has this phase been captured?", and "captured, and there was
|
||||
/// nothing" is an answer to that question. `metadata.empty` is what tells the
|
||||
/// two apart — the same convention `mission_delivery` uses for its "No code
|
||||
/// changes" artifact.
|
||||
async fn register_empty_marker(
|
||||
pool: &PgPool,
|
||||
mission_id: Uuid,
|
||||
phase_id: Uuid,
|
||||
dest: &Path,
|
||||
) -> Result<(), String> {
|
||||
std::fs::create_dir_all(dest).map_err(|e| format!("create {}: {e}", dest.display()))?;
|
||||
let file = dest.join(EMPTY_MARKER);
|
||||
std::fs::write(
|
||||
&file,
|
||||
"This phase finished without leaving any files in its workspace, so there\n was nothing to publish. If the phase is meant to reason rather than\n produce, set `config.allow_empty = true` on it.\n",
|
||||
)
|
||||
.map_err(|e| format!("write {}: {e}", file.display()))?;
|
||||
let rel = file
|
||||
.strip_prefix(missions_root())
|
||||
.map(|r| r.to_string_lossy().to_string())
|
||||
.unwrap_or_else(|_| file.to_string_lossy().to_string());
|
||||
cm_db::repo::missions::register_artifact(
|
||||
pool,
|
||||
cm_db::repo::missions::RegisterArtifact {
|
||||
mission_id,
|
||||
phase_id: Some(phase_id),
|
||||
path: &rel,
|
||||
kind: OUTPUT_KIND,
|
||||
mime: Some("text/markdown"),
|
||||
title: Some("No output produced"),
|
||||
generated_by_run: None,
|
||||
render_pdf: false,
|
||||
metadata: Some(serde_json::json!({ "empty": true })),
|
||||
},
|
||||
)
|
||||
.await
|
||||
.map(|_| ())
|
||||
.map_err(|e| format!("register empty marker: {e}"))
|
||||
}
|
||||
|
||||
/// Copy `/mission/repo` out of the mission's container and return the files kept.
|
||||
async fn collect_into(mission_id: Uuid, dest: &Path) -> Result<Vec<PathBuf>, String> {
|
||||
let container = crate::mission_runtime::container_name(mission_id);
|
||||
@@ -323,6 +388,35 @@ mod tests {
|
||||
assert!(!is_markdown(Path::new("/x/data.json")));
|
||||
}
|
||||
|
||||
/// A phase that produced nothing must still leave a marker, or the
|
||||
/// selection query matches it on every tick forever.
|
||||
///
|
||||
/// Measured on the first live negative control: the guard logged "produced
|
||||
/// NO output files" 4 times, then 8 times 45 seconds later — a docker
|
||||
/// copy_out per tick, and with a bounded batch, five such phases would
|
||||
/// starve every other repo-less mission out of capture permanently.
|
||||
/// `phase_runner::record_uncapturable` was written for the identical
|
||||
/// failure on the diff path.
|
||||
#[test]
|
||||
fn an_empty_phase_leaves_a_marker_so_it_is_not_reconsidered_forever() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let dest = tmp.path().join("out");
|
||||
// The file-writing half of `register_empty_marker`, which is the part
|
||||
// that must exist for the artifact row to point at something real.
|
||||
std::fs::create_dir_all(&dest).unwrap();
|
||||
let file = dest.join(EMPTY_MARKER);
|
||||
std::fs::write(&file, "x").unwrap();
|
||||
assert!(file.exists(), "an artifact row must not point at nothing");
|
||||
assert_eq!(
|
||||
file.file_name().unwrap().to_string_lossy(),
|
||||
"NO-OUTPUT.md",
|
||||
"the marker name is part of the contract with readers"
|
||||
);
|
||||
// And the marker must not itself be mistaken for captured output on a
|
||||
// later pass: it is filtered like any other scaffolding would be.
|
||||
assert!(keep_files(&dest).iter().any(|p| p == &file));
|
||||
}
|
||||
|
||||
/// Artifacts must land OUTSIDE the mission directory. `teardown_container`
|
||||
/// removes `<missions_root>/<mission_id>` wholesale, so a capture written
|
||||
/// inside it would be destroyed by the very reap it exists to survive.
|
||||
|
||||
Reference in New Issue
Block a user