Skip to content

Commit 7cd9d27

Browse files
chubes4Chris Huber
andauthored
fix(agent-task): isolate fanout liveness attribution (#11939)
AI assistance: OpenAI gpt-5.6-sol via OpenCode inspected the concurrent lifecycle and process activity paths, implemented the per-provider PID attribution, and added deterministic regression tests. Chris Huber remains responsible for this change. Co-authored-by: Chris Huber <chris@chubes.net>
1 parent d4d9ca3 commit 7cd9d27

8 files changed

Lines changed: 235 additions & 16 deletions

File tree

crates/homeboy-agents/src/agent_task_lifecycle/lifecycle_ops.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1661,6 +1661,52 @@ pub fn reserve_provider_execution(
16611661
Ok(reservation)
16621662
}
16631663

1664+
/// Bind a reserved provider execution to the subprocess that actually runs it.
1665+
///
1666+
/// Fanout workers are threads and therefore share the coordinator PID. The
1667+
/// provider subprocess is the first process identity unique to one child run;
1668+
/// using it keeps liveness and activity evidence from crossing child records.
1669+
pub fn record_provider_execution_process(
1670+
run_id: &str,
1671+
task_id: &str,
1672+
attempt: u32,
1673+
pid: u32,
1674+
) -> Result<AgentTaskRunRecord> {
1675+
let run_id = sanitize_run_id(run_id);
1676+
let key = format!("{task_id}:{attempt}");
1677+
let record = store::mutate_record(&run_id, |record| {
1678+
let Some(execution) = record.metadata["provider_executions"]
1679+
.as_array_mut()
1680+
.and_then(|executions| {
1681+
executions
1682+
.iter_mut()
1683+
.find(|execution| execution["key"] == key)
1684+
})
1685+
else {
1686+
return false;
1687+
};
1688+
execution["owner_pid"] = json!(pid);
1689+
execution["owner_linux_starttime_ticks"] =
1690+
json!(homeboy_core::process::linux_process_starttime_ticks(pid)
1691+
.ok()
1692+
.flatten());
1693+
true
1694+
})?;
1695+
record.ok_or_else(|| {
1696+
Error::validation_invalid_argument(
1697+
"provider_execution",
1698+
"cannot bind a process to an unreserved provider execution",
1699+
Some(key),
1700+
None,
1701+
)
1702+
})
1703+
}
1704+
1705+
/// Return the unambiguous running provider PID for activity sampling.
1706+
pub fn running_owner_pid(run_id: &str) -> Result<Option<u32>> {
1707+
Ok(store::read_record(&sanitize_run_id(run_id))?.owner_pid())
1708+
}
1709+
16641710
/// Persist the controller-owned Cook phase independently of provider output.
16651711
/// This gives foreground observers a restart-safe liveness source without
16661712
/// treating an arbitrary provider transcript line as durable state.

crates/homeboy-agents/src/agent_task_lifecycle/records.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,6 +623,22 @@ impl AgentTaskRunRecord {
623623
}
624624

625625
pub(crate) fn owner_pid(&self) -> Option<u32> {
626+
let provider_pids = self
627+
.metadata
628+
.get("provider_executions")
629+
.and_then(Value::as_array)
630+
.into_iter()
631+
.flatten()
632+
.filter(|execution| execution["state"] == "running")
633+
.filter_map(|execution| {
634+
execution["owner_pid"]
635+
.as_u64()
636+
.and_then(|pid| u32::try_from(pid).ok())
637+
})
638+
.collect::<Vec<_>>();
639+
if provider_pids.len() == 1 {
640+
return provider_pids.into_iter().next();
641+
}
626642
self.metadata
627643
.get("runner_pid")
628644
.and_then(Value::as_u64)

crates/homeboy-agents/src/agent_task_provider/command_runner.rs

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,18 @@ pub(super) fn run_materialized_provider_command(
5353
request: &AgentTaskExecutorRequest,
5454
provider: &AgentTaskExecutorProvider,
5555
run_id: Option<&str>,
56+
execution_attempt: u32,
5657
) -> AgentTaskOutcome {
57-
let mut attempt = 1;
58+
let mut retry_attempt = 1;
5859
loop {
59-
let mut outcome = run_materialized_provider_command_once(request, provider);
60+
let mut outcome =
61+
run_materialized_provider_command_once(request, provider, run_id, execution_attempt);
6062
classify_transient_provider_outcome(&mut outcome);
6163

6264
let retryable = outcome_is_transient(&outcome);
63-
if !retryable || attempt >= PROVIDER_TRANSIENT_MAX_ATTEMPTS {
64-
if attempt > 1 {
65-
annotate_transient_retry(&mut outcome, attempt, retryable);
65+
if !retryable || retry_attempt >= PROVIDER_TRANSIENT_MAX_ATTEMPTS {
66+
if retry_attempt > 1 {
67+
annotate_transient_retry(&mut outcome, retry_attempt, retryable);
6668
}
6769
attach_runtime_tool_provenance(request, &mut outcome);
6870
// Preserve and link the latest raw executor input/result as
@@ -71,11 +73,12 @@ pub(super) fn run_materialized_provider_command(
7173
return outcome;
7274
}
7375

74-
let backoff_ms = PROVIDER_TRANSIENT_BASE_BACKOFF_MS.saturating_mul(1u64 << (attempt - 1));
76+
let backoff_ms =
77+
PROVIDER_TRANSIENT_BASE_BACKOFF_MS.saturating_mul(1u64 << (retry_attempt - 1));
7578
if backoff_ms > 0 {
7679
std::thread::sleep(Duration::from_millis(backoff_ms));
7780
}
78-
attempt += 1;
81+
retry_attempt += 1;
7982
}
8083
}
8184

@@ -315,12 +318,16 @@ impl ProviderContainmentReport {
315318
pub(super) fn run_materialized_provider_command_once(
316319
request: &AgentTaskExecutorRequest,
317320
provider: &AgentTaskExecutorProvider,
321+
run_id: Option<&str>,
322+
attempt: u32,
318323
) -> AgentTaskOutcome {
319324
let mut containment_report = ProviderContainmentReport::default();
320325
let mut outcome = run_materialized_provider_command_once_contained(
321326
request,
322327
provider,
323328
&mut containment_report,
329+
run_id,
330+
attempt,
324331
);
325332
containment_report.annotate(&mut outcome);
326333
if let Err(error) = retain_failed_workspace_artifacts(&mut outcome, request) {
@@ -339,6 +346,8 @@ fn run_materialized_provider_command_once_contained(
339346
request: &AgentTaskExecutorRequest,
340347
provider: &AgentTaskExecutorProvider,
341348
containment_report: &mut ProviderContainmentReport,
349+
run_id: Option<&str>,
350+
attempt: u32,
342351
) -> AgentTaskOutcome {
343352
let command = render_provider_command_display(provider);
344353
let deadline_remaining_ms = crate::agent_task_timeout::remaining_execution_deadline_ms(
@@ -554,6 +563,17 @@ fn run_materialized_provider_command_once_contained(
554563
}
555564
};
556565

566+
if let Some(run_id) = run_id {
567+
// Failure to record this diagnostic identity must not interrupt provider
568+
// execution. The reservation remains the execution authority.
569+
let _ = crate::agent_task_lifecycle::record_provider_execution_process(
570+
run_id,
571+
&request.request.task_id,
572+
attempt,
573+
child.id(),
574+
);
575+
}
576+
557577
if let Err(error) = containment.attach(&child) {
558578
let attach_error = error.to_string();
559579
let cleanup = containment.terminate_live(&mut child);
@@ -1084,7 +1104,7 @@ pub(super) fn run_provider_command(
10841104
run_id: Option<&str>,
10851105
) -> AgentTaskOutcome {
10861106
let materialized = test_executor_request(request);
1087-
run_materialized_provider_command(&materialized, provider, run_id)
1107+
run_materialized_provider_command(&materialized, provider, run_id, 1)
10881108
}
10891109

10901110
#[cfg(test)]
@@ -1093,7 +1113,7 @@ pub(super) fn run_provider_command_once(
10931113
provider: &AgentTaskExecutorProvider,
10941114
) -> AgentTaskOutcome {
10951115
let materialized = test_executor_request(request);
1096-
run_materialized_provider_command_once(&materialized, provider)
1116+
run_materialized_provider_command_once(&materialized, provider, None, 1)
10971117
}
10981118

10991119
#[cfg(test)]

crates/homeboy-agents/src/agent_task_provider/executor.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,12 @@ impl AgentTaskExecutorAdapter for ExtensionProviderAgentTaskExecutor {
211211
&ready_tools,
212212
));
213213

214-
run_materialized_provider_command(&request, &provider, context.run_id.as_deref())
214+
run_materialized_provider_command(
215+
&request,
216+
&provider,
217+
context.run_id.as_deref(),
218+
context.attempt,
219+
)
215220
}
216221
}
217222

crates/homeboy-agents/src/agent_task_service/cook.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3705,7 +3705,13 @@ where
37053705
while let Err(mpsc::RecvTimeoutError::Timeout) =
37063706
heartbeat_wait.recv_timeout(COOK_HEARTBEAT_INTERVAL)
37073707
{
3708-
let activity = heartbeat_activity.sample(heartbeat_owner_pid);
3708+
let activity_owner_pid = agent_task_lifecycle::running_owner_pid(
3709+
&heartbeat_run_id,
3710+
)
3711+
.ok()
3712+
.flatten()
3713+
.unwrap_or(heartbeat_owner_pid);
3714+
let activity = heartbeat_activity.sample(activity_owner_pid);
37093715
let tick = supervisor.observe(&activity);
37103716
let detail = tick.detail_line();
37113717
let _ = report_cook_progress_with_activity(

crates/homeboy-agents/src/agent_task_service/discovery.rs

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,13 @@ fn discovery_run(
552552
let last_update_age_minutes = age_minutes(last_update.as_deref(), now);
553553
let source = run_source(&record);
554554
let liveness = classify.then(|| classify_liveness(&record, last_update_age_minutes, now));
555+
let stale_reason = metadata_string(
556+
&record.metadata,
557+
agent_task_lifecycle::METADATA_KEY_STALE_RUNNING_REASON,
558+
)
559+
.or_else(|| {
560+
(liveness == Some(AgentTaskLiveness::Stale)).then(|| stale_reason_for_record(&record))
561+
});
555562

556563
// Runner metadata describes execution placement, not necessarily lifecycle
557564
// record ownership. Controller handoff projections retain their durable
@@ -584,11 +591,12 @@ fn discovery_run(
584591
runner_id,
585592
runner_job_id,
586593
remote_run_id: metadata_string(&record.metadata, "remote_run_id"),
587-
stale: metadata_bool(&record.metadata, agent_task_lifecycle::METADATA_KEY_STALE_RUNNING),
588-
stale_reason: metadata_string(
589-
&record.metadata,
590-
agent_task_lifecycle::METADATA_KEY_STALE_RUNNING_REASON,
591-
),
594+
stale: (liveness == Some(AgentTaskLiveness::Stale))
595+
.then_some(true)
596+
.or_else(|| {
597+
metadata_bool(&record.metadata, agent_task_lifecycle::METADATA_KEY_STALE_RUNNING)
598+
}),
599+
stale_reason,
592600
retryable: metadata_bool(&record.metadata, agent_task_lifecycle::METADATA_KEY_RETRYABLE),
593601
liveness,
594602
liveness_reconcilable: liveness.map(AgentTaskLiveness::is_reconcilable),
@@ -613,6 +621,21 @@ fn discovery_run(
613621
}
614622
}
615623

624+
/// Explain every stale read projection, including a pure discovery read that
625+
/// deliberately leaves the durable record untouched.
626+
fn stale_reason_for_record(record: &AgentTaskRunRecord) -> String {
627+
if record.owner_pid().is_some() && !record.owner_process_is_running() {
628+
return "owner_process_not_running".to_string();
629+
}
630+
if record.runner_job_id().is_some() {
631+
return "runner_job_unverified_after_daemon_restart".to_string();
632+
}
633+
if record.owner_pid().is_none() {
634+
return "missing_runner_pid".to_string();
635+
}
636+
"stale_running_marked".to_string()
637+
}
638+
616639
fn discovery_counts(tasks: &[agent_task_lifecycle::AgentTaskRunTask]) -> AgentTaskDiscoveryCounts {
617640
let mut counts = AgentTaskDiscoveryCounts::default();
618641
for task in tasks {

crates/homeboy-agents/src/agent_task_service/process_activity.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -631,6 +631,34 @@ mod tests {
631631
assert_eq!(activity.rss_kib, Some(262_144));
632632
}
633633

634+
#[test]
635+
fn concurrent_provider_roots_do_not_cross_attribute_activity_or_resources() {
636+
// Both provider children share a coordinator parent. Sampling from that
637+
// parent would blend their commands and resource trees; each child PID
638+
// must be used as the root instead.
639+
let rows = parse_process_activity_rows(concat!(
640+
"1 0 S 1024 10:00 homeboy agent-task fanout cook\n",
641+
"101 1 S 2048 05:00 opencode run child-a\n",
642+
"102 101 S 4096 04:00 cargo test child-a\n",
643+
"201 1 S 8192 05:00 opencode run child-b\n",
644+
"202 201 S 16384 04:00 cargo test child-b\n",
645+
));
646+
647+
let first = select_provider_activity(&rows, 101, &[]);
648+
let second = select_provider_activity(&rows, 201, &[]);
649+
650+
assert_eq!(
651+
first.activity.expect("first activity").command,
652+
"cargo test child-a"
653+
);
654+
assert_eq!(
655+
second.activity.expect("second activity").command,
656+
"cargo test child-b"
657+
);
658+
assert_eq!(first.tree_rss_kib, Some(4_096));
659+
assert_eq!(second.tree_rss_kib, Some(16_384));
660+
}
661+
634662
#[test]
635663
fn an_unmeasured_tree_reports_no_memory_rather_than_zero() {
636664
// A budget that fired on a fabricated zero would be worse than no

crates/homeboy-agents/src/agent_task_service/tests.rs

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1541,6 +1541,81 @@ fn dead_owner_process_run_is_classified_stale_and_reconciled() {
15411541
});
15421542
}
15431543

1544+
#[test]
1545+
fn concurrent_provider_children_keep_liveness_attributed_to_their_own_processes() {
1546+
with_isolated_home(|_| {
1547+
let mut child_a = std::process::Command::new("sleep")
1548+
.arg("60")
1549+
.spawn()
1550+
.expect("start first provider child");
1551+
let mut child_b = std::process::Command::new("sleep")
1552+
.arg("60")
1553+
.spawn()
1554+
.expect("start second provider child");
1555+
1556+
let result = (|| {
1557+
for (run_id, pid) in [("run-child-a", child_a.id()), ("run-child-b", child_b.id())] {
1558+
let plan = discovery_plan();
1559+
agent_task_lifecycle::submit_plan(&plan, Some(run_id)).expect("submit child");
1560+
agent_task_lifecycle::mark_running(run_id).expect("mark child running");
1561+
agent_task_lifecycle::reserve_provider_execution(run_id, &plan.tasks[0], 1)
1562+
.expect("reserve provider execution");
1563+
agent_task_lifecycle::record_provider_execution_process(
1564+
run_id,
1565+
&plan.tasks[0].task_id,
1566+
1,
1567+
pid,
1568+
)
1569+
.expect("bind provider process");
1570+
}
1571+
1572+
let active =
1573+
discover_runs(AgentTaskDiscoveryFilter::Active).expect("discover children");
1574+
assert!(active
1575+
.runs
1576+
.iter()
1577+
.all(|run| run.liveness == Some(AgentTaskLiveness::Active)));
1578+
1579+
child_a.kill().expect("kill first provider child");
1580+
child_a.wait().expect("reap first provider child");
1581+
let observed =
1582+
discover_runs(AgentTaskDiscoveryFilter::Active).expect("rediscover children");
1583+
assert_eq!(
1584+
observed
1585+
.runs
1586+
.iter()
1587+
.find(|run| run.run_id == "run-child-a")
1588+
.expect("first child")
1589+
.liveness,
1590+
Some(AgentTaskLiveness::Stale)
1591+
);
1592+
assert_eq!(
1593+
observed
1594+
.runs
1595+
.iter()
1596+
.find(|run| run.run_id == "run-child-a")
1597+
.expect("first child")
1598+
.stale_reason
1599+
.as_deref(),
1600+
Some("owner_process_not_running")
1601+
);
1602+
assert_eq!(
1603+
observed
1604+
.runs
1605+
.iter()
1606+
.find(|run| run.run_id == "run-child-b")
1607+
.expect("second child")
1608+
.liveness,
1609+
Some(AgentTaskLiveness::Active)
1610+
);
1611+
})();
1612+
1613+
let _ = child_b.kill();
1614+
let _ = child_b.wait();
1615+
result
1616+
});
1617+
}
1618+
15441619
#[test]
15451620
fn discovery_latest_returns_only_newest_run() {
15461621
with_isolated_home(|_| {

0 commit comments

Comments
 (0)