Skip to content

Commit a839418

Browse files
committed
fix(server): purge a retired request's step manifest
A deferred-matrix node is dispatched as a placeholder and gets a manifest seeded, then retire_node_requests(Purge) removes the request when expansion replaces it with real legs. The manifest stayed, so every dynamic expansion accumulated an entry no run projection could reach. It is now dropped alongside the request's other correlation state. Durable rows cannot leak the same way: dispatch-time seeding is not persisted and a placeholder never reports, so it never reaches store_job_steps; rows for attempts that did report go with their run through the runs foreign key. The test asserts every retained manifest belongs to a live request, and fails with the removal commented out. Reported on #212 by Codex and Cubic.
1 parent c820e51 commit a839418

2 files changed

Lines changed: 89 additions & 0 deletions

File tree

‎crates/preloop-runner-server/src/lib_tests.rs‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2843,6 +2843,84 @@ async fn log_run_logs_unknown_job_is_404_not_whole_run() {
28432843
);
28442844
}
28452845

2846+
/// Expansion does not leave the placeholder's manifest behind.
2847+
///
2848+
/// A deferred-matrix node is dispatched as a placeholder, gets a manifest
2849+
/// seeded, and is purged when expansion replaces it with real legs. Without
2850+
/// cleanup every dynamic expansion accumulates an entry no run projection can
2851+
/// reach.
2852+
#[tokio::test]
2853+
async fn expansion_purges_the_placeholder_step_manifest() {
2854+
let temp = tempfile::tempdir().unwrap();
2855+
let state = AppState::new(temp.path().to_path_buf()).await.unwrap();
2856+
let app = app(state.clone(), CancellationToken::new());
2857+
2858+
let accepted = submit_yaml(
2859+
&app,
2860+
r#"
2861+
on: push
2862+
jobs:
2863+
gen:
2864+
runs-on: ubuntu-latest
2865+
outputs:
2866+
matrix: ${{ steps.build.outputs.matrix }}
2867+
steps:
2868+
- id: build
2869+
run: echo matrix
2870+
fan:
2871+
needs: [gen]
2872+
runs-on: ubuntu-latest
2873+
strategy:
2874+
matrix: ${{ fromJson(needs.gen.outputs.matrix) }}
2875+
steps:
2876+
- run: echo leg
2877+
"#,
2878+
"owner/repo",
2879+
)
2880+
.await;
2881+
let run_id: RunId = accepted["run_id"].as_str().unwrap().parse().unwrap();
2882+
2883+
let before = {
2884+
let inner = state.inner.lock().await;
2885+
inner.job_steps.len()
2886+
};
2887+
2888+
// Finish `gen` with a matrix so the deferred node expands.
2889+
let _ = crate::distributed_task::complete_job_inner(
2890+
state.shared(),
2891+
preloop_gha_protocol::JobCompletion {
2892+
run_id,
2893+
job_id: preloop_gha_protocol::JobId("gen".to_owned()),
2894+
agent_job_id: None,
2895+
status: ExecutionStatus::Success,
2896+
outputs: [("matrix".to_owned(), serde_json::json!("{\"leg\":[1,2]}"))]
2897+
.into_iter()
2898+
.collect(),
2899+
annotations: Vec::new(),
2900+
step_results: Vec::new(),
2901+
},
2902+
)
2903+
.await;
2904+
2905+
let inner = state.inner.lock().await;
2906+
// Every retained manifest must belong to a request that still exists.
2907+
let live: std::collections::BTreeSet<uuid::Uuid> = inner
2908+
.job_requests
2909+
.values()
2910+
.map(|record| record.agent_job_id)
2911+
.collect();
2912+
let orphans: Vec<&uuid::Uuid> = inner
2913+
.job_steps
2914+
.keys()
2915+
.filter(|agent_job_id| !live.contains(agent_job_id))
2916+
.collect();
2917+
assert!(
2918+
orphans.is_empty(),
2919+
"expansion left {} unreachable manifest(s) (had {before} before)",
2920+
orphans.len()
2921+
);
2922+
}
2923+
28462924
/// The list endpoint projects steps like the single-run endpoint.
28472925
///
28482926
/// Step records live in the attempt manifest, not in the stored run, so a

‎crates/preloop-runner-server/src/runtime_scheduling.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2678,6 +2678,17 @@ pub(crate) fn retire_node_requests(
26782678
inner.live_log_lines.remove(&agent_key);
26792679
inner.live_log_tx.remove(&agent_key);
26802680
inner.live_log_closed.remove(&agent_key);
2681+
// The step manifest is attempt-scoped, so it belongs to the
2682+
// request being purged. A deferred-matrix placeholder gets one
2683+
// seeded at dispatch and is then purged when expansion
2684+
// replaces it, so without this every dynamic expansion leaves
2685+
// an entry no run projection can reach.
2686+
//
2687+
// Durable rows cannot leak the same way: seeding is not
2688+
// persisted, and a placeholder never reports, so it never
2689+
// reaches `store_job_steps`. Rows for attempts that did report
2690+
// are removed with their run through the `runs` foreign key.
2691+
inner.job_steps.remove(&record.agent_job_id);
26812692
}
26822693
}
26832694
}

0 commit comments

Comments
 (0)