Skip to content

Commit a1b811a

Browse files
author
Chris Huber
committed
fix(runner): bound recovery churn (#11957)
AI assistance: OpenAI GPT-5.6 Sol via OpenCode implemented and tested the bounded recovery convergence. Chris Huber remains responsible for every line.
1 parent f9ea056 commit a1b811a

5 files changed

Lines changed: 205 additions & 72 deletions

File tree

crates/homeboy-cli/src/cli_runtime.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -775,13 +775,13 @@ fn schedule_runner_exec_recovery() {
775775
let Ok(Some(schedule)) = crate::runner::schedule_terminal_runner_exec_recovery() else {
776776
return;
777777
};
778-
eprintln!(
779-
"runner-exec recovery accepted: owner_id={} deferred_count={} budget_ms={} inspect=`{}`",
780-
schedule.owner_id, schedule.deferred_count, schedule.budget_ms, schedule.inspection_action
781-
);
782778
if !schedule.is_new_owner {
783779
return;
784780
}
781+
eprintln!(
782+
"runner-exec recovery scheduled: owner_id={} deferred_count={} inspect=`{}`",
783+
schedule.owner_id, schedule.deferred_count, schedule.inspection_action
784+
);
785785
let executable = match std::env::current_exe() {
786786
Ok(executable) => executable,
787787
Err(error) => {

crates/homeboy-cli/src/command_capability.rs

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,29 +149,15 @@ pub fn classify(args: &[String]) -> CommandCapability {
149149
}
150150
}
151151

152-
/// Whether startup may reconcile runner-exec state before this invocation.
152+
/// Whether this command owns runner-exec recovery.
153153
///
154-
/// This is intentionally narrower than command mutation: a review may
155-
/// materialize a controller-local artifact, but runner recovery would make its
156-
/// already-durable aggregate unavailable during a Lab outage.
154+
/// Recovery may contact a previously selected runner, so only `runner exec`
155+
/// may start it. Other mutations must not turn routine commands into an
156+
/// unrelated background-recovery admission path.
157157
pub fn requires_startup_reconciliation(args: &[String]) -> bool {
158-
classify(args) == CommandCapability::Mutation
159-
// The lifecycle command that launches these internal processes already
160-
// performs startup recovery. Repeating it here delays lease publication
161-
// and can make the supervisor kill an otherwise healthy cold start.
162-
&& !args
163-
.get(1..)
164-
.unwrap_or_default()
165-
.windows(2)
166-
.any(|args| {
167-
matches!(args, [command, subcommand]
168-
if command == "daemon" && matches!(subcommand.as_str(), "supervise" | "serve"))
169-
})
170-
&& !args
171-
.get(1..)
172-
.unwrap_or_default()
173-
.windows(2)
174-
.any(|args| args == ["agent-task", "review"])
158+
homeboy_owned_args(args.get(1..).unwrap_or_default())
159+
.windows(2)
160+
.any(|args| args == ["runner", "exec"])
175161
}
176162

177163
#[cfg(test)]
@@ -255,7 +241,7 @@ mod tests {
255241
}
256242

257243
#[test]
258-
fn review_keeps_its_local_materialization_capability_but_skips_runner_recovery() {
244+
fn only_runner_exec_owns_startup_recovery() {
259245
let review = args(&[
260246
"homeboy",
261247
"agent-task",
@@ -266,17 +252,20 @@ mod tests {
266252
]);
267253
assert_eq!(classify(&review), CommandCapability::Mutation);
268254
assert!(!requires_startup_reconciliation(&review));
269-
assert!(requires_startup_reconciliation(&args(&[
255+
assert!(!requires_startup_reconciliation(&args(&[
270256
"homeboy",
271257
"agent-task",
272258
"retry",
273259
"run-1",
274260
"--run",
275261
])));
262+
assert!(requires_startup_reconciliation(&args(&[
263+
"homeboy", "runner", "exec", "lab", "--", "true",
264+
])));
276265
}
277266

278267
#[test]
279-
fn internal_daemon_processes_publish_their_lease_without_repeating_recovery() {
268+
fn other_mutations_do_not_own_runner_recovery() {
280269
for command in [
281270
args(&[
282271
"homeboy",
@@ -305,7 +294,7 @@ mod tests {
305294
args(&["homeboy", "daemon", "start"]),
306295
args(&["homeboy", "daemon", "ensure-running"]),
307296
] {
308-
assert!(requires_startup_reconciliation(&command));
297+
assert!(!requires_startup_reconciliation(&command));
309298
}
310299
}
311300

crates/homeboy-core/src/observation/store/runs.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,25 @@ impl ObservationStore {
322322
Ok(rows == 1)
323323
}
324324

325+
/// Persist a retryable recovery blocker only while this child still owns the
326+
/// source lease. The replacement metadata deliberately releases the lease
327+
/// so a later, bounded retry can make progress.
328+
pub fn defer_running_runner_exec_recovery_source(
329+
&self,
330+
run_id: &str,
331+
child_token: &str,
332+
metadata_json: serde_json::Value,
333+
) -> Result<bool> {
334+
let metadata_json = serialize_metadata(&metadata_json)?;
335+
let rows = execute_with_retry("defer claimed runner recovery source", || {
336+
self.connection.execute(
337+
"UPDATE runs SET metadata_json = ?1 WHERE id = ?2 AND status = 'running' AND json_extract(metadata_json, '$.runner_exec_source_lease.source_token') = ?3",
338+
params![metadata_json, run_id, child_token],
339+
)
340+
})?;
341+
Ok(rows == 1)
342+
}
343+
325344
pub fn start_run_with_context(
326345
&self,
327346
run: NewRunRecord,

crates/homeboy-lab-runner/src/execution/recovery.rs

Lines changed: 148 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ const RECOVERY_OWNER_ID: &str = "runner-exec-recovery";
1919
const RECOVERY_CHILD_KIND: &str = "runner_exec_recovery_child";
2020
const RECOVERY_OWNER_LEASE: Duration = Duration::from_secs(30);
2121
const RECOVERY_LEASE_HEARTBEAT: Duration = Duration::from_secs(1);
22+
const MAX_SOURCE_RECOVERY_DEFERRALS: u64 = 3;
2223
#[derive(Clone)]
2324
struct RecoveryWorker {
2425
id: String,
@@ -223,7 +224,9 @@ fn reconcile_terminal_runner_exec_runs_with_owner(
223224
if error.details.get("http_status").and_then(Value::as_u64) != Some(404) {
224225
unavailable_endpoints.insert(endpoint);
225226
}
226-
deferred += 1;
227+
if defer_or_fail_recovery_source(&store, run, &worker.token, job_id, &error)? {
228+
deferred += 1;
229+
}
227230
continue;
228231
}
229232
};
@@ -337,6 +340,54 @@ fn reconcile_terminal_runner_exec_runs_with_owner(
337340
Ok((reconciled, deferred))
338341
}
339342

343+
/// A runner that remains unavailable cannot be retried by every unrelated
344+
/// command forever. Keep a small, inspectable retry budget, then terminalize
345+
/// the source with the exact blocker retained in its durable metadata.
346+
fn defer_or_fail_recovery_source(
347+
store: &ObservationStore,
348+
run: &homeboy_core::observation::RunRecord,
349+
child_token: &str,
350+
runner_job_id: &str,
351+
error: &Error,
352+
) -> Result<bool> {
353+
let attempts = run
354+
.metadata_json
355+
.pointer("/runner_exec_recovery/deferral_count")
356+
.and_then(Value::as_u64)
357+
.unwrap_or(0)
358+
+ 1;
359+
let mut metadata = run.metadata_json.clone();
360+
metadata.as_object_mut().map(|metadata| {
361+
metadata.remove("runner_exec_source_lease");
362+
});
363+
metadata["runner_exec_recovery"] = json!({
364+
"schema": "homeboy/runner-exec-recovery/v1",
365+
"phase": if attempts >= MAX_SOURCE_RECOVERY_DEFERRALS { "blocked" } else { "deferred" },
366+
"deferral_count": attempts,
367+
"max_deferrals": MAX_SOURCE_RECOVERY_DEFERRALS,
368+
"reason": error.message,
369+
"details": error.details,
370+
"inspection_action": format!("homeboy runs show {}", run.id),
371+
});
372+
if attempts >= MAX_SOURCE_RECOVERY_DEFERRALS {
373+
metadata["runner_terminal_projection"] = json!({
374+
"state": "recovery_blocked",
375+
"classification": "runner_unavailable_after_bounded_recovery",
376+
"runner_id": run.metadata_json["runner_id"],
377+
"runner_job_id": runner_job_id,
378+
});
379+
store.fail_running_runner_exec_recovery_source(
380+
&run.id,
381+
child_token,
382+
runner_job_id,
383+
metadata,
384+
)?;
385+
return Ok(false);
386+
}
387+
store.defer_running_runner_exec_recovery_source(&run.id, child_token, metadata)?;
388+
Ok(true)
389+
}
390+
340391
fn endpoint_identity(session: &crate::RunnerSession) -> String {
341392
session
342393
.remote_daemon_address
@@ -1012,6 +1063,102 @@ mod tests {
10121063
});
10131064
}
10141065

1066+
#[test]
1067+
fn repeated_deferred_recovery_converges_without_stale_children() {
1068+
with_isolated_home(|_| {
1069+
let run_id = "unavailable-source";
1070+
let runner_job_id = "unavailable-job";
1071+
homeboy_agents::agent_task_lifecycle::record_runner_exec_job_identity(
1072+
run_id,
1073+
"unavailable-runner",
1074+
runner_job_id,
1075+
"/workspace",
1076+
&[],
1077+
)
1078+
.expect("source");
1079+
let store = ObservationStore::open_initialized().expect("store");
1080+
let error = Error::validation_invalid_argument(
1081+
"runner",
1082+
"runner has no persisted daemon session for recovery",
1083+
Some("unavailable-runner".to_string()),
1084+
None,
1085+
);
1086+
1087+
let mut child_ids = BTreeSet::new();
1088+
for attempt in 1..=MAX_SOURCE_RECOVERY_DEFERRALS {
1089+
let owner = schedule_terminal_runner_exec_recovery()
1090+
.expect("schedule")
1091+
.expect("owner");
1092+
let work = run_scheduled_terminal_runner_exec_recovery(
1093+
&owner.owner_id,
1094+
&owner.owner_token,
1095+
)
1096+
.expect("schedule child")
1097+
.expect("owner work");
1098+
assert_eq!(work.children.len(), 1);
1099+
let child = &work.children[0];
1100+
child_ids.insert(child.child_id.clone());
1101+
let source = store.get_run(run_id).expect("read").expect("source");
1102+
assert_eq!(
1103+
defer_or_fail_recovery_source(
1104+
&store,
1105+
&source,
1106+
&child.child_token,
1107+
runner_job_id,
1108+
&error,
1109+
)
1110+
.expect("record bounded deferral"),
1111+
attempt < MAX_SOURCE_RECOVERY_DEFERRALS,
1112+
);
1113+
let child_record = store
1114+
.get_run(&child.child_id)
1115+
.expect("read")
1116+
.expect("child");
1117+
store
1118+
.finish_running_run_with_owner_token(
1119+
&child.child_id,
1120+
&child.child_token,
1121+
RunStatus::Pass,
1122+
child_record.metadata_json,
1123+
)
1124+
.expect("finish child");
1125+
finish_scheduled_terminal_runner_exec_recovery(
1126+
&owner.owner_id,
1127+
&owner.owner_token,
1128+
1,
1129+
0,
1130+
usize::from(attempt < MAX_SOURCE_RECOVERY_DEFERRALS),
1131+
)
1132+
.expect("finish owner");
1133+
}
1134+
1135+
let source = store.get_run(run_id).expect("read").expect("source");
1136+
assert_eq!(source.status, RunStatus::Fail.as_str());
1137+
assert_eq!(
1138+
source.metadata_json["runner_exec_recovery"]["deferral_count"],
1139+
MAX_SOURCE_RECOVERY_DEFERRALS
1140+
);
1141+
assert_eq!(
1142+
source.metadata_json["runner_terminal_projection"]["classification"],
1143+
"runner_unavailable_after_bounded_recovery"
1144+
);
1145+
let reader = ObservationStore::open_scheduler_reader().expect("reader");
1146+
assert!(recovery_candidates(&reader).expect("candidates").is_empty());
1147+
let children = store
1148+
.list_runs(RunListFilter {
1149+
kind: Some(RECOVERY_CHILD_KIND.to_string()),
1150+
..RunListFilter::default()
1151+
})
1152+
.expect("list children");
1153+
assert_eq!(child_ids.len(), 1, "retries reuse one child identity");
1154+
assert_eq!(children.len(), 1, "retries do not accumulate children");
1155+
assert_ne!(children[0].status, RunStatus::Running.as_str());
1156+
assert!(schedule_terminal_runner_exec_recovery()
1157+
.expect("final schedule")
1158+
.is_none());
1159+
});
1160+
}
1161+
10151162
#[test]
10161163
fn owner_schedules_one_durable_child_per_source_within_its_budget() {
10171164
with_isolated_home(|_| {

tests/cook_lab_handoff_test.rs

Lines changed: 20 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use std::path::Path;
22
use std::process::Output;
33
use std::time::{Duration, Instant};
44

5-
use homeboy_core::observation::{NewRunRecord, ObservationStore, RunListFilter, RunStatus};
5+
use homeboy_core::observation::{NewRunRecord, ObservationStore, RunListFilter};
66
use homeboy_core::test_support::{bounded_output, HermeticTestContext, TestBinary};
77

88
/// Run the fixture binary through the shared hermetic harness.
@@ -292,11 +292,11 @@ fn non_tty_local_wait_stays_foreground() {
292292
);
293293
}
294294

295-
/// Historical runner recovery is a distinct owner, never an admission gate for
296-
/// an accepted Cook. This invokes the actual detached Cook command path rather
297-
/// than a scheduler helper so its output and durable handoff remain observable.
295+
/// Historical runner recovery belongs to `runner exec`, never an unrelated
296+
/// Cook admission. This invokes the actual detached Cook command path so the
297+
/// ownership boundary remains observable.
298298
#[test]
299-
fn detached_cook_admission_is_bounded_with_a_hundred_unavailable_recovery_records() {
299+
fn detached_cook_admission_does_not_schedule_unrelated_recovery_records() {
300300
let context = HermeticTestContext::new();
301301
let database = context.data_dir().join("homeboy.sqlite");
302302
let store = ObservationStore::open_initialized_at(&database).expect("open fixture store");
@@ -354,43 +354,21 @@ fn detached_cook_admission_is_bounded_with_a_hundred_unavailable_recovery_record
354354
assert_eq!(handoff["detached"], true, "{stdout}");
355355
assert!(handoff["cook_id"].as_str().is_some_and(|id| !id.is_empty()));
356356

357-
let observation_deadline = Instant::now() + Duration::from_secs(5);
358-
let (owners, children) = loop {
359-
let store = ObservationStore::open_initialized_at(&database).expect("reopen fixture store");
360-
let owners = store
361-
.list_runs(RunListFilter {
362-
kind: Some("runner_exec_recovery".to_string()),
363-
..RunListFilter::default()
364-
})
365-
.expect("list recovery owners");
366-
let children = store
367-
.list_runs(RunListFilter {
368-
kind: Some("runner_exec_recovery_child".to_string()),
369-
..RunListFilter::default()
370-
})
371-
.expect("list recovery children");
372-
if owners.len() == 1
373-
&& owners[0].status != RunStatus::Running.as_str()
374-
&& children.len() == 100
375-
&& children
376-
.iter()
377-
.all(|child| child.status != RunStatus::Running.as_str())
378-
{
379-
break (owners, children);
380-
}
381-
assert!(
382-
Instant::now() < observation_deadline,
383-
"recovery state did not settle"
384-
);
385-
std::thread::sleep(Duration::from_millis(25));
386-
};
387-
assert_eq!(owners.len(), 1, "recovery has its own durable owner");
388-
assert_eq!(owners[0].status, RunStatus::Pass.as_str());
389-
assert_eq!(owners[0].metadata_json["scheduled_count"], 100);
390-
assert_eq!(children.len(), 100, "each source has a durable child");
391-
assert!(children
392-
.iter()
393-
.all(|child| child.status == RunStatus::Pass.as_str()));
357+
let store = ObservationStore::open_initialized_at(&database).expect("reopen fixture store");
358+
assert!(store
359+
.list_runs(RunListFilter {
360+
kind: Some("runner_exec_recovery".to_string()),
361+
..RunListFilter::default()
362+
})
363+
.expect("list recovery owners")
364+
.is_empty());
365+
assert!(store
366+
.list_runs(RunListFilter {
367+
kind: Some("runner_exec_recovery_child".to_string()),
368+
..RunListFilter::default()
369+
})
370+
.expect("list recovery children")
371+
.is_empty());
394372

395373
#[cfg(unix)]
396374
if let Some(pid) = handoff["pid"].as_u64() {

0 commit comments

Comments
 (0)