Skip to content

Commit caa7f19

Browse files
authored
fix(test): wait for durable state instead of racing the daemon supervisor (#11927)
Four daemon tests failed on main. Three raced a documented asynchronous contract; one repeated the path drift fixed in #11919. `POST /controller/jobs/{id}/cancel` is asynchronous by design. `request_controller_cancellation` persists the intent and only terminalizes inline when the job is *claimless*; a running job is transitioned by the supervisor thread spawned in `daemon::mod`. `LocalControllerJobClient::cancel` states it outright: it persists a request and returns "the daemon's current job projection" while "the controller continues provider shutdown asynchronously". Asserting the terminal state immediately after the request is therefore a race that only passes on an idle machine, and the cancel response body is a snapshot at request time rather than the outcome. Add a bounded `wait_for` helper and poll the durable state: - controller_jobs_are_durable_idempotent_and_fail_closed_after_restart - generic_cancel_rejects_running_controller_work_without_touching_its_driver - controller_job_errors_use_driver_safe_public_projection The no-leak assertions in the third are unchanged and still assert on a body fetched after the projection lands. Also convert four fixed-iteration poll loops (0..50 or 0..100 at 5-10ms, i.e. a 250-500ms budget) to the same helper, and fix route_serves_run_scoped_artifact_store_tokens, which hardcoded <home>/.local/share/homeboy/artifacts while HOMEBOY_DATA_DIR points the reader elsewhere -- the fourth instance of the drift #11919 documented.
1 parent af32b4f commit caa7f19

2 files changed

Lines changed: 68 additions & 36 deletions

File tree

tests/core/daemon/artifact_download_test.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ fn test_route() {
3333
#[test]
3434
fn route_serves_run_scoped_artifact_store_tokens() {
3535
let _home = HomeGuard::new();
36-
let home_path = std::path::PathBuf::from(std::env::var("HOME").expect("home"));
3736
let store = ObservationStore::open_initialized().expect("store");
3837
let run = store
3938
.start_run(
@@ -45,7 +44,11 @@ fn route_serves_run_scoped_artifact_store_tokens() {
4544
.expect("run");
4645
let locator =
4746
"homeboy/workflow-bench/runs/run-1/artifacts/scenario/adapter/attempt-1/summary.json";
48-
let artifact_root = home_path.join(".local/share/homeboy/artifacts");
47+
// Ask the resolver. `HomeGuard` sets `HOMEBOY_DATA_DIR`, which
48+
// `homeboy_data()` checks before the XDG fallback, so this run's artifacts
49+
// do not live under `<home>/.local/share`. Same drift that broke
50+
// `artifact_content_serves_encoded_artifact_store_locator` (#11919).
51+
let artifact_root = crate::paths::artifact_root().expect("artifact root");
4952
let path = artifact_root.join(locator);
5053
fs::create_dir_all(path.parent().expect("artifact parent")).expect("artifact parent");
5154
fs::write(&path, br#"{"ok":true}"#).expect("artifact-store file");

tests/core/daemon_test.rs

Lines changed: 63 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,31 @@ impl ControllerJobDriver for FailingCancellationControllerDriver {
311311
}
312312
}
313313

314+
/// Wait for a durable condition instead of asserting immediately after a
315+
/// cancellation request.
316+
///
317+
/// `POST /controller/jobs/{id}/cancel` is **asynchronous by contract**:
318+
/// `request_controller_cancellation` persists the intent and only terminalizes
319+
/// inline when the job is *claimless*. A running job is transitioned by the
320+
/// supervisor thread spawned in `daemon::mod`, and
321+
/// `LocalControllerJobClient::cancel` documents that it "persist[s] a
322+
/// cancellation request and return[s] the daemon's current job projection"
323+
/// while "the controller continues provider shutdown asynchronously".
324+
///
325+
/// Asserting the terminal state straight after the request is therefore a race
326+
/// that only passes on an idle machine. These tests lost it consistently on a
327+
/// loaded host.
328+
fn wait_for(label: &str, mut condition: impl FnMut() -> bool) {
329+
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10);
330+
while std::time::Instant::now() < deadline {
331+
if condition() {
332+
return;
333+
}
334+
std::thread::sleep(std::time::Duration::from_millis(5));
335+
}
336+
panic!("timed out waiting for {label}");
337+
}
338+
314339
#[test]
315340
fn controller_jobs_are_durable_idempotent_and_fail_closed_after_restart() {
316341
let _home = HomeGuard::new();
@@ -399,12 +424,10 @@ fn controller_jobs_are_durable_idempotent_and_fail_closed_after_restart() {
399424
assert_eq!(inline_secret.status_code, 400);
400425

401426
release_tx.send(()).expect("release blocked driver");
402-
for _ in 0..50 {
403-
if store.get(job_id).expect("first job").status.is_terminal() {
404-
break;
405-
}
406-
std::thread::sleep(std::time::Duration::from_millis(10));
407-
}
427+
wait_for(
428+
"store.get(job_id).expect( first job ).status.is_terminal()",
429+
|| store.get(job_id).expect("first job").status.is_terminal(),
430+
);
408431

409432
let cancelled = route_with_body(
410433
"POST",
@@ -437,17 +460,16 @@ fn controller_jobs_are_durable_idempotent_and_fail_closed_after_restart() {
437460
&store,
438461
);
439462
assert_eq!(cancelled.status_code, 200);
440-
assert_eq!(cancelled.body["body"]["job"]["status"], "cancelled");
441-
assert_eq!(
442-
store.get(cancelled_id).expect("cancelled job").status,
443-
JobStatus::Cancelled
444-
);
445-
for _ in 0..50 {
446-
if cancellations.load(Ordering::SeqCst) == 1 {
447-
break;
448-
}
449-
std::thread::sleep(std::time::Duration::from_millis(10));
450-
}
463+
// The response is the projection at request time, not the terminal state.
464+
wait_for("cancelled_id to reach Cancelled", || {
465+
store
466+
.get(cancelled_id)
467+
.map(|job| job.status == JobStatus::Cancelled)
468+
.unwrap_or(false)
469+
});
470+
wait_for("cancellations.load(Ordering::SeqCst) == 1", || {
471+
cancellations.load(Ordering::SeqCst) == 1
472+
});
451473
assert_eq!(cancellations.load(Ordering::SeqCst), 1);
452474
let repeated_cancel = route_with_body(
453475
"POST",
@@ -776,12 +798,16 @@ fn generic_cancel_rejects_running_controller_work_without_touching_its_driver()
776798
&store,
777799
);
778800
assert_eq!(cancelled.status_code, 200);
779-
assert_eq!(
780-
store.get(job_id).expect("cancelled job").status,
781-
JobStatus::Cancelled
782-
);
801+
wait_for("controller job to reach Cancelled", || {
802+
store
803+
.get(job_id)
804+
.map(|job| job.status == JobStatus::Cancelled)
805+
.unwrap_or(false)
806+
});
783807
assert_eq!(executions.load(Ordering::SeqCst), 1);
784-
assert_eq!(cancellations.load(Ordering::SeqCst), 1);
808+
wait_for("driver cancel to be observed", || {
809+
cancellations.load(Ordering::SeqCst) == 1
810+
});
785811
}
786812

787813
#[test]
@@ -1232,12 +1258,9 @@ fn controller_job_retry_after_compaction_returns_tombstoned_terminal_projection(
12321258
assert!(store
12331259
.get(uuid::Uuid::parse_str(&first_id).expect("valid ID"))
12341260
.is_err());
1235-
for _ in 0..100 {
1236-
if controller_job_runtime_count() == 0 {
1237-
break;
1238-
}
1239-
std::thread::sleep(std::time::Duration::from_millis(5));
1240-
}
1261+
wait_for("controller_job_runtime_count() == 0", || {
1262+
controller_job_runtime_count() == 0
1263+
});
12411264
assert_eq!(controller_job_runtime_count(), 0);
12421265
let executions_before_retry = executions.load(Ordering::SeqCst);
12431266
let retry = route_with_body("POST", "/controller/jobs", Some(request), &store);
@@ -1341,6 +1364,14 @@ fn controller_job_errors_use_driver_safe_public_projection() {
13411364
None,
13421365
&store,
13431366
);
1367+
// The driver's failing `cancel` is projected by the supervisor thread, so
1368+
// poll for the safe classification rather than racing it.
1369+
wait_for("safe cancellation failure projection", || {
1370+
serialized_contains(
1371+
&route_with_body("GET", &format!("/jobs/{job_id}/events"), None, &store).body,
1372+
"safe_driver_failure",
1373+
)
1374+
});
13441375
let events = route_with_body("GET", &format!("/jobs/{job_id}/events"), None, &store);
13451376
assert!(!serialized_contains(
13461377
&response.body,
@@ -2732,12 +2763,10 @@ fn cancelling_daemon_exec_job_terminates_process_tree() {
27322763
uuid::Uuid::parse_str(response.body["body"]["job"]["id"].as_str().expect("job id"))
27332764
.expect("parse job id");
27342765

2735-
for _ in 0..50 {
2736-
if store.get(job_id).expect("job").status == JobStatus::Running {
2737-
break;
2738-
}
2739-
std::thread::sleep(std::time::Duration::from_millis(20));
2740-
}
2766+
wait_for(
2767+
"store.get(job_id).expect( job ).status == JobStatus::Running",
2768+
|| store.get(job_id).expect("job").status == JobStatus::Running,
2769+
);
27412770
store
27422771
.cancel(job_id, "test cancellation")
27432772
.expect("cancel job");

0 commit comments

Comments
 (0)