From 954add9ebd6f3c05bd0abad2f83d21bb682a92c1 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 18:04:56 +0200 Subject: [PATCH 01/14] kernel: dispatch runnable steps in parallel Session-Id: 01a062cc-f525-7d01-932e-a634815114c1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine.rs | 21 ++- .../src/machine/parallel_tests.rs | 141 ++++++++++++++++++ 2 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 kernel/relayflowd-core/src/machine/parallel_tests.rs diff --git a/kernel/relayflowd-core/src/machine.rs b/kernel/relayflowd-core/src/machine.rs index 6e97ca952..812505bfb 100644 --- a/kernel/relayflowd-core/src/machine.rs +++ b/kernel/relayflowd-core/src/machine.rs @@ -106,6 +106,24 @@ pub fn next_actions(state: &RunState, now_ms: i64) -> Vec { return complete_run_actions(state, RunCompletionReason::Success, None, now_ms); } + // A fold marks every dependency-free step Runnable at once. Preserve the + // authored spec order while emitting every journal-first start pair from + // that one state snapshot; dependencies unlocked by these executions are + // considered only after their completions are folded on the next pass. + let starts = state + .spec + .steps + .iter() + .filter_map(|spec| { + let runtime = &state.steps[&spec.id]; + (runtime.state == StepState::Runnable).then_some((spec, runtime)) + }) + .flat_map(|(spec, runtime)| start_actions(state, spec, runtime.attempts + 1, now_ms)) + .collect::>(); + if !starts.is_empty() { + return starts; + } + let mut timers = Vec::new(); for spec in &state.spec.steps { let runtime = &state.steps[&spec.id]; @@ -130,7 +148,6 @@ pub fn next_actions(state: &RunState, now_ms: i64) -> Vec { StepState::Backoff { wake_at_ms, .. } => { timers.push(Action::ArmTimer { at_ms: wake_at_ms }); } - StepState::Runnable => return start_actions(state, spec, runtime.attempts + 1, now_ms), _ => {} } } @@ -423,5 +440,7 @@ fn deterministic_ulid( mod recovery; pub use recovery::{abandonment_actions, recovery_actions, recovery_actions_filtered}; +#[cfg(test)] +mod parallel_tests; #[cfg(test)] mod tests; diff --git a/kernel/relayflowd-core/src/machine/parallel_tests.rs b/kernel/relayflowd-core/src/machine/parallel_tests.rs new file mode 100644 index 000000000..a0bc518e3 --- /dev/null +++ b/kernel/relayflowd-core/src/machine/parallel_tests.rs @@ -0,0 +1,141 @@ +use serde_json::json; + +use super::*; +use crate::{entry::StepCompletedPayload, state::RunState}; + +fn parallel_spec() -> crate::RunSpec { + crate::RunSpec::parse(&json!({ + "steps": [ + { + "id": "lane-b", + "type": "llm", + "prompt": "research b", + "retry": {"initial_backoff_ms": 0, "max_backoff_ms": 0, "multiplier": 1, "jitter_percent": 0} + }, + { + "id": "join", + "type": "deterministic", + "command": "true", + "depends_on": ["lane-a", "lane-b"] + }, + { + "id": "lane-a", + "type": "llm", + "prompt": "research a", + "retry": {"initial_backoff_ms": 0, "max_backoff_ms": 0, "multiplier": 1, "jitter_percent": 0} + } + ] + })) + .unwrap() +} + +fn appended_entries(actions: &[Action]) -> Vec { + actions + .iter() + .filter_map(|action| match action { + Action::Append(entry) => Some(entry.clone()), + _ => None, + }) + .collect() +} + +#[test] +fn machine_starts_every_runnable_step_in_authored_order() { + let state = RunState::fold("run", parallel_spec(), &[]).unwrap(); + let actions = next_actions(&state, 10); + + assert_eq!( + actions, + next_actions(&state, 10), + "the same journal state and simulated time must emit the same batch" + ); + assert_eq!(actions.len(), 4, "both independent lanes must start"); + for (pair, expected_step) in actions.chunks_exact(2).zip(["lane-b", "lane-a"]) { + let Action::Append(started) = &pair[0] else { + panic!("each lease must be journaled before dispatch") + }; + assert_eq!(started.entry_type, EntryType::StepAttemptStarted); + assert_eq!(started.step_id.as_deref(), Some(expected_step)); + let Action::Dispatch { step, attempt, .. } = &pair[1] else { + panic!("each independent llm lane must dispatch") + }; + assert_eq!(step.id, expected_step); + assert_eq!(*attempt, 1); + } +} + +#[test] +fn parallel_lanes_do_not_cross_the_dependency_barrier_early() { + let spec = parallel_spec(); + let fresh = RunState::fold("run", spec.clone(), &[]).unwrap(); + let mut entries = appended_entries(&next_actions(&fresh, 10)); + assert_eq!(entries.len(), 2, "both lane leases must be journaled"); + + let complete = |step: &crate::StepSpec, answer: &str| { + completion_actions( + "run", + step, + 1, + 0, + AttemptResult::successful(json!({"answer": answer}), "worker"), + 20, + ) + .into_iter() + .find_map(|action| match action { + Action::Append(entry) if entry.entry_type == EntryType::StepCompleted => Some(entry), + _ => None, + }) + .unwrap() + }; + + entries.push(complete(&spec.steps[0], "b")); + let one_lane_running = RunState::fold("run", spec.clone(), &entries).unwrap(); + assert!(next_actions(&one_lane_running, 20).is_empty()); + assert_eq!(one_lane_running.steps["join"].state, StepState::Pending); + + entries.push(complete(&spec.steps[2], "a")); + let both_lanes_done = RunState::fold("run", spec, &entries).unwrap(); + let actions = next_actions(&both_lanes_done, 20); + let Action::Append(join_started) = &actions[0] else { + panic!("the join must start once every dependency succeeds") + }; + assert_eq!(join_started.step_id.as_deref(), Some("join")); +} + +#[test] +fn crash_resume_preserves_each_parallel_lease_exactly_once() { + let spec = parallel_spec(); + let fresh = RunState::fold("run", spec.clone(), &[]).unwrap(); + let actions = next_actions(&fresh, 10); + let starts = appended_entries(&actions); + assert_eq!(starts.len(), 2, "both parallel leases must be durable"); + + // Crash after only the first journal append: resume schedules the lane + // whose lease was never persisted, without re-emitting the durable one. + let partial = RunState::fold("run", spec.clone(), &starts[..1]).unwrap(); + let resumed = next_actions(&partial, 11); + assert_eq!(resumed.len(), 2); + let Action::Append(resumed_start) = &resumed[0] else { + panic!("the unstarted lane must journal its lease") + }; + assert_eq!(resumed_start.step_id.as_deref(), Some("lane-a")); + + // Crash after both journal appends: recovery explains both in-flight + // attempts in the same deterministic order, once each. + let running = RunState::fold("run", spec, &starts).unwrap(); + let recovered = recovery_actions(&running, 12); + let completions = recovered + .iter() + .filter_map(|action| match action { + Action::Append(entry) if entry.entry_type == EntryType::StepCompleted => Some(entry), + _ => None, + }) + .collect::>(); + assert_eq!(completions.len(), 2); + assert_eq!(completions[0].step_id.as_deref(), Some("lane-b")); + assert_eq!(completions[1].step_id.as_deref(), Some("lane-a")); + for entry in completions { + let payload: StepCompletedPayload = serde_json::from_value(entry.payload.clone()).unwrap(); + assert_eq!(payload.completion_reason, CompletionReason::Crashed); + } +} From 1b99dacad778e7aa1c515e4a8f7bbdd0a5643a8a Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 19:02:48 +0200 Subject: [PATCH 02/14] fix(kernel): drive complete parallel dispatch batches Session-Id: 01a062cc-f525-7d01-932e-a634815114c1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine.rs | 61 ++- .../src/machine/parallel_tests.rs | 28 +- kernel/relayflowd/src/engine/drive.rs | 192 ++++---- kernel/relayflowd/src/server.rs | 9 +- kernel/relayflowd/src/server/session.rs | 46 +- .../tests/crash_resume/concurrency.rs | 135 +++++- .../tests/crash_resume/llm_support.rs | 37 ++ kernel/relayflowd/tests/parallel_driver.rs | 413 ++++++++++++++++++ 8 files changed, 805 insertions(+), 116 deletions(-) create mode 100644 kernel/relayflowd/tests/parallel_driver.rs diff --git a/kernel/relayflowd-core/src/machine.rs b/kernel/relayflowd-core/src/machine.rs index 812505bfb..7ff694524 100644 --- a/kernel/relayflowd-core/src/machine.rs +++ b/kernel/relayflowd-core/src/machine.rs @@ -106,6 +106,43 @@ pub fn next_actions(state: &RunState, now_ms: i64) -> Vec { return complete_run_actions(state, RunCompletionReason::Success, None, now_ms); } + // Wake every retry whose deterministic timer is due before starting work. + // Recovery can put several crashed parallel lanes into the same zero-delay + // backoff; waking just one would let an already-runnable peer start and park + // the run while the other due lane remained asleep. + let due_waits = state + .spec + .steps + .iter() + .filter_map(|spec| { + let runtime = &state.steps[&spec.id]; + let StepState::Backoff { + attempt, + wake_at_ms, + } = runtime.state + else { + return None; + }; + (wake_at_ms <= now_ms).then(|| { + Action::Append(JournalEntry::new( + EntryType::WaitCompleted, + state.run_id.clone(), + Some(spec.id.clone()), + Some(attempt), + now_ms, + WaitCompletedPayload { + wait_id: retry_wait_id(&state.run_id, &spec.id, attempt, wake_at_ms), + completion_reason: WaitCompletionReason::TimerFired, + result: Value::Null, + }, + )) + }) + }) + .collect::>(); + if !due_waits.is_empty() { + return due_waits; + } + // A fold marks every dependency-free step Runnable at once. Preserve the // authored spec order while emitting every journal-first start pair from // that one state snapshot; dependencies unlocked by these executions are @@ -127,28 +164,8 @@ pub fn next_actions(state: &RunState, now_ms: i64) -> Vec { let mut timers = Vec::new(); for spec in &state.spec.steps { let runtime = &state.steps[&spec.id]; - match runtime.state { - StepState::Backoff { - attempt, - wake_at_ms, - } if wake_at_ms <= now_ms => { - return vec![Action::Append(JournalEntry::new( - EntryType::WaitCompleted, - state.run_id.clone(), - Some(spec.id.clone()), - Some(attempt), - now_ms, - WaitCompletedPayload { - wait_id: retry_wait_id(&state.run_id, &spec.id, attempt, wake_at_ms), - completion_reason: WaitCompletionReason::TimerFired, - result: Value::Null, - }, - ))]; - } - StepState::Backoff { wake_at_ms, .. } => { - timers.push(Action::ArmTimer { at_ms: wake_at_ms }); - } - _ => {} + if let StepState::Backoff { wake_at_ms, .. } = runtime.state { + timers.push(Action::ArmTimer { at_ms: wake_at_ms }); } } timers.sort_by_key(|action| match action { diff --git a/kernel/relayflowd-core/src/machine/parallel_tests.rs b/kernel/relayflowd-core/src/machine/parallel_tests.rs index a0bc518e3..8340d1839 100644 --- a/kernel/relayflowd-core/src/machine/parallel_tests.rs +++ b/kernel/relayflowd-core/src/machine/parallel_tests.rs @@ -122,7 +122,7 @@ fn crash_resume_preserves_each_parallel_lease_exactly_once() { // Crash after both journal appends: recovery explains both in-flight // attempts in the same deterministic order, once each. - let running = RunState::fold("run", spec, &starts).unwrap(); + let running = RunState::fold("run", spec.clone(), &starts).unwrap(); let recovered = recovery_actions(&running, 12); let completions = recovered .iter() @@ -138,4 +138,30 @@ fn crash_resume_preserves_each_parallel_lease_exactly_once() { let payload: StepCompletedPayload = serde_json::from_value(entry.payload.clone()).unwrap(); assert_eq!(payload.completion_reason, CompletionReason::Crashed); } + + let mut entries = starts; + entries.extend(appended_entries(&recovered)); + let backoff = RunState::fold("run", spec.clone(), &entries).unwrap(); + let due_waits = next_actions(&backoff, 12); + assert_eq!( + due_waits.len(), + 2, + "both recovered lanes must wake together" + ); + assert!(due_waits.iter().all(|action| matches!( + action, + Action::Append(entry) if entry.entry_type == EntryType::WaitCompleted + ))); + + entries.extend(appended_entries(&due_waits)); + let runnable = RunState::fold("run", spec, &entries).unwrap(); + let restarted = next_actions(&runnable, 12); + assert_eq!(restarted.len(), 4, "both recovered lanes must restart"); + for (pair, expected_step) in restarted.chunks_exact(2).zip(["lane-b", "lane-a"]) { + let Action::Append(started) = &pair[0] else { + panic!("each recovered lease must be journaled before dispatch") + }; + assert_eq!(started.step_id.as_deref(), Some(expected_step)); + assert_eq!(started.attempt, Some(2)); + } } diff --git a/kernel/relayflowd/src/engine/drive.rs b/kernel/relayflowd/src/engine/drive.rs index bb3dbf4d7..d02c6d3cd 100644 --- a/kernel/relayflowd/src/engine/drive.rs +++ b/kernel/relayflowd/src/engine/drive.rs @@ -1,9 +1,9 @@ -use std::{thread, time::Duration}; +use std::{collections::BTreeSet, thread, time::Duration}; use anyhow::{Result, bail}; use relayflowd_core::{ Action, AttemptResult, Clock, CompletionReason, RunCompletionReason, RunSpec, RunState, - completion_actions, next_actions, + abandonment_actions, completion_actions, next_actions, }; use relayflowd_journal::SqliteJournal; @@ -34,31 +34,52 @@ impl Engine { .set_status(&state.run_id, "interrupted", None)?; return Ok(parked_outcome(&state, RunStatus::Interrupted)); } - if !pause_consumed && should_pause(&state, &options) { + if !pause_consumed && options.pause_before_completion && state.all_steps_succeeded() { pause_consumed = true; thread::sleep(Duration::from_secs(300)); } - // No worker can take the next out-of-band step: park. `parked` means - // "nothing is coming until something changes"; `waiting_worker` means - // "a worker holds this lease" and makes `resume` block for it. An - // agent worker that cannot pin every surface the step declares is not - // a compatible worker either — parking keeps the failure a declared - // state instead of an untyped error raised after `run.start`. - if let Some(step) = runnable_out_of_band_step(&state) - && !self.step_is_dispatchable(&state, step) - { - self.registry()?.set_status(&state.run_id, "parked", None)?; - return Ok(parked_outcome(&state, RunStatus::Parked)); - } let actions = next_actions(&state, self.clock.now_ms()); if actions.is_empty() { self.park_idle_run(&state)?; return Ok(parked_outcome(&state, RunStatus::Parked)); } + let mut dispatched = false; + let mut backpressured = false; + let mut skipped_dispatches = BTreeSet::new(); for action in actions { match action { Action::Append(mut entry) => { + if entry.entry_type == relayflowd_core::EntryType::StepAttemptStarted { + let step_id = entry + .step_id + .as_deref() + .expect("a step start always names its step"); + let step = state + .spec + .step(step_id) + .expect("the scheduler only starts declared steps"); + if !pause_consumed + && options.pause_before_step.as_deref() == Some(step_id) + { + pause_consumed = true; + thread::sleep(Duration::from_secs(300)); + } + if step.step_type() != relayflowd_core::StepType::Deterministic + && !self.step_is_dispatchable(&state, step) + { + // Admission is per pair. A lane with no + // compatible worker remains Runnable, while + // later independent lanes still get their + // journal-first handoff. + skipped_dispatches.insert(( + step_id.to_owned(), + entry.attempt.expect("a step start has an attempt"), + )); + backpressured = true; + continue; + } + } self.prepare_start_entry(&state, &mut entry)?; self.assign_executor(&mut entry)?; self.append(&mut journal, &entry)?; @@ -76,6 +97,17 @@ impl Engine { ) { self.interpret_non_execution(&mut journal, action)?; } + let completed = self.load_state(&journal, spec.clone())?; + if options.stop_after.is_some_and(|limit| { + completed + .completed_steps() + .saturating_sub(initial_completed) + >= limit + }) { + self.registry()? + .set_status(&completed.run_id, "interrupted", None)?; + return Ok(parked_outcome(&completed, RunStatus::Interrupted)); + } } Action::Dispatch { step, @@ -87,6 +119,9 @@ impl Engine { pins: _, recovery, } => { + if skipped_dispatches.remove(&(step.id.clone(), attempt)) { + continue; + } let started_state = self.load_state(&journal, spec.clone())?; let pins = started_state.steps[&step.id] .last_start_pins @@ -114,31 +149,51 @@ impl Engine { }) .transpose()? .unwrap_or(DispatchOutcome::NoWorker); - // Appendix A rule 2: a worker standing at revisions - // other than the attempt's pins cannot start from the - // journaled state. The attempt fails closed with a - // declared reason instead of running against pins - // nothing holds, and the step re-elects on the retry. - if let DispatchOutcome::PinMismatch { detail } = outcome { - self.fail_dispatch_closed( - &mut journal, - &started_state, - &step, - attempt, - detail, - )?; - break; + match outcome { + DispatchOutcome::Dispatched => { + // Record operational backpressure after each + // handoff. If the driver crashes before the + // rest of the batch, the durable start entry + // and this wake deadline still describe the + // lease already handed out. + self.registry()?.set_status( + &state.run_id, + "waiting_worker", + Some(lease_deadline_ms), + )?; + dispatched = true; + } + DispatchOutcome::NoWorker => { + // Preflight admitted the whole batch, so this + // is a detach race. Explain the unhanded lease + // as crashed, leave it retryable, and continue: + // one lost worker must not discard later batch + // actions that another worker can honor. + for action in abandonment_actions( + &started_state, + &step.id, + attempt, + CompletionReason::Crashed, + self.clock.now_ms(), + ) { + self.persist_only(&mut journal, action)?; + } + backpressured = true; + } + DispatchOutcome::PinMismatch { detail } => { + // Appendix A rule 2: a worker standing at + // revisions other than the journaled pins + // fails closed. Continue the batch so an + // independent compatible lane is not dropped. + self.fail_dispatch_closed( + &mut journal, + &started_state, + &step, + attempt, + detail, + )?; + } } - self.registry()?.set_status( - &state.run_id, - if outcome == DispatchOutcome::Dispatched { - "waiting_worker" - } else { - "parked" - }, - Some(lease_deadline_ms), - )?; - return Ok(parked_outcome(&state, RunStatus::Parked)); } Action::ArmTimer { at_ms } => { self.wait_for_timer(&journal, at_ms)?; @@ -159,6 +214,11 @@ impl Engine { } } } + if dispatched || backpressured { + let parked = self.load_state(&journal, spec.clone())?; + self.park_idle_run(&parked)?; + return Ok(parked_outcome(&parked, RunStatus::Parked)); + } } } @@ -244,19 +304,19 @@ impl Engine { } fn park_idle_run(&self, state: &RunState) -> Result<()> { - let active_lease = - state - .spec - .steps - .iter() - .find_map(|step| match state.steps[&step.id].state { - relayflowd_core::StepState::Running { - lease_deadline_ms, .. - } if step.step_type() != relayflowd_core::StepType::Deterministic => { - Some(lease_deadline_ms) - } - _ => None, - }); + let active_lease = state + .spec + .steps + .iter() + .filter_map(|step| match state.steps[&step.id].state { + relayflowd_core::StepState::Running { + lease_deadline_ms, .. + } if step.step_type() != relayflowd_core::StepType::Deterministic => { + Some(lease_deadline_ms) + } + _ => None, + }) + .min(); match active_lease { Some(deadline_ms) => { self.registry()? @@ -276,31 +336,3 @@ fn parked_outcome(state: &RunState, status: RunStatus) -> RunOutcome { completed_steps: state.completed_steps(), } } - -/// The step `next_actions` will start next, when it needs an out-of-band -/// worker. `next_actions` starts at most one step per pass, so there is at most -/// one such step to preflight. -fn runnable_out_of_band_step(state: &RunState) -> Option<&relayflowd_core::StepSpec> { - state.spec.steps.iter().find(|step| { - matches!( - state.steps[&step.id].state, - relayflowd_core::StepState::Runnable - ) && step.step_type() != relayflowd_core::StepType::Deterministic - }) -} - -fn should_pause(state: &RunState, options: &DriveOptions) -> bool { - if options.pause_before_completion && state.all_steps_succeeded() { - return true; - } - let Some(step_id) = options.pause_before_step.as_deref() else { - return false; - }; - state.spec.steps.iter().find_map(|step| { - matches!( - state.steps[&step.id].state, - relayflowd_core::StepState::Runnable - ) - .then_some(step.id.as_str()) - }) == Some(step_id) -} diff --git a/kernel/relayflowd/src/server.rs b/kernel/relayflowd/src/server.rs index 491463bce..eb5cd8ee2 100644 --- a/kernel/relayflowd/src/server.rs +++ b/kernel/relayflowd/src/server.rs @@ -243,7 +243,7 @@ fn handle_request( } "step.heartbeat" => { let params: StepHeartbeatParams = decode_params(request.params)?; - let deadline = hub + let (deadline, run_deadline) = hub .heartbeat( connection_id, &(params.run_id.clone(), params.step_id, params.attempt), @@ -256,7 +256,7 @@ fn handle_request( // write fails the heartbeat — the worker must not believe its // lease was extended when nothing durable says so. engine - .renew_lease(¶ms.run_id, deadline) + .renew_lease(¶ms.run_id, run_deadline) .map_err(internal_error)?; Ok(json!({"lease_deadline_ms": deadline})) } @@ -310,6 +310,11 @@ fn handle_request( ) .map_err(internal_error)?; hub.finish(&key); + if let Some(deadline) = hub.earliest_lease_deadline(¶ms.run_id) { + engine + .renew_lease(¶ms.run_id, deadline) + .map_err(internal_error)?; + } to_value(outcome) } "effect.record" => { diff --git a/kernel/relayflowd/src/server/session.rs b/kernel/relayflowd/src/server/session.rs index 58d440bd3..6bbff6e80 100644 --- a/kernel/relayflowd/src/server/session.rs +++ b/kernel/relayflowd/src/server/session.rs @@ -214,17 +214,27 @@ impl ProtocolHub { key: &AssignmentKey, lease_id: &str, now_ms: i64, - ) -> Result { + ) -> Result<(i64, i64)> { let mut sessions = self.sessions.lock().expect("protocol sessions lock"); - let assignment = sessions + let assignment_deadline = { + let assignment = sessions + .assignments + .get_mut(key) + .context("attempt has no active worker lease")?; + if assignment.connection_id != connection_id || assignment.lease_id != lease_id { + bail!("heartbeat does not match the active worker lease") + } + assignment.lease_deadline_ms = now_ms.saturating_add(LEASE_RENEWAL_MS); + assignment.lease_deadline_ms + }; + let run_deadline = sessions .assignments - .get_mut(key) - .context("attempt has no active worker lease")?; - if assignment.connection_id != connection_id || assignment.lease_id != lease_id { - bail!("heartbeat does not match the active worker lease") - } - assignment.lease_deadline_ms = now_ms.saturating_add(LEASE_RENEWAL_MS); - Ok(assignment.lease_deadline_ms) + .iter() + .filter(|((run_id, _, _), _)| run_id == &key.0) + .map(|(_, assignment)| assignment.lease_deadline_ms) + .min() + .expect("the renewed assignment is still present"); + Ok((assignment_deadline, run_deadline)) } pub fn completion_worker(&self, connection_id: u64, key: &AssignmentKey) -> Result { @@ -248,7 +258,9 @@ impl ProtocolHub { } /// Release every in-memory lease after the journal has durably made the - /// run terminal. Calling this repeatedly is intentionally harmless. + /// run terminal. Calling this repeatedly is intentionally harmless. Under + /// parallel dispatch a terminal run can hold several live leases at once, + /// so this releases the whole run rather than one attempt. pub fn finish_run(&self, run_id: &str) { self.sessions .lock() @@ -257,6 +269,20 @@ impl ProtocolHub { .retain(|(assigned_run, _, _), _| assigned_run != run_id); } + /// The run registry has one operational wake deadline even when the + /// journal has several live leases. It must track the earliest assignment + /// so a heartbeat on one lane cannot hide an earlier sibling expiry. + pub fn earliest_lease_deadline(&self, run_id: &str) -> Option { + self.sessions + .lock() + .expect("protocol sessions lock") + .assignments + .iter() + .filter(|((assigned_run, _, _), _)| assigned_run == run_id) + .map(|(_, assignment)| assignment.lease_deadline_ms) + .min() + } + /// Assignments whose (heartbeat-renewed) lease deadline has passed. The /// worker may still hold an open socket — a hung worker is exactly the /// case the expiry reconciler exists for. Assignments are NOT removed diff --git a/kernel/relayflowd/tests/crash_resume/concurrency.rs b/kernel/relayflowd/tests/crash_resume/concurrency.rs index 6a14b5863..4986f6f63 100644 --- a/kernel/relayflowd/tests/crash_resume/concurrency.rs +++ b/kernel/relayflowd/tests/crash_resume/concurrency.rs @@ -5,18 +5,151 @@ use std::{ sync::{Arc, Barrier}, thread, + time::Duration, }; use relayflowd_core::{ CompletionReason, EntryType, RunCompletedPayload, RunCompletionReason, StepCompletedPayload, }; +use relayflowd_journal::Registry; use serde_json::json; use super::{ - llm_support::{LlmFixture, ProtocolClient, ServerGuard, attached_worker, complete, start_run}, + llm_support::{ + LlmFixture, ProtocolClient, ServerGuard, attached_worker, complete, spawn_resume, start_run, + }, support::journal_entries, }; +#[test] +fn run_start_dispatches_every_independent_lane_before_any_completion() { + let fixture = LlmFixture::parallel("socket-fan-out"); + let _server = ServerGuard::start(&fixture); + let mut worker = attached_worker(&fixture, "parallel-stub"); + let run_id = start_run(&fixture); + + worker.set_read_timeout(Some(Duration::from_secs(1))); + let first = worker.event("step.dispatch").unwrap(); + let second = worker + .event("step.dispatch") + .expect("both independent lanes must dispatch before either completes"); + worker.set_read_timeout(None); + assert_eq!(first["step_id"], "lane-b"); + assert_eq!(second["step_id"], "lane-a"); + + let original_deadline = second["lease_deadline_ms"].as_i64().unwrap(); + let heartbeat = worker + .request( + "step.heartbeat", + json!({ + "run_id": run_id, + "step_id": first["step_id"], + "attempt": first["attempt"], + "lease_id": first["lease_id"] + }), + ) + .unwrap(); + let renewed_deadline = heartbeat["lease_deadline_ms"].as_i64().unwrap(); + assert!(renewed_deadline >= original_deadline); + let registry = Registry::open(fixture.data_dir.join("relayflowd.sqlite3")).unwrap(); + assert_eq!( + registry.lookup(&run_id).unwrap().unwrap().next_wake_at_ms, + Some(original_deadline), + "one lane's heartbeat must not hide its sibling's earlier deadline" + ); + + // A live resume sees both leases held and must neither abandon nor + // redispatch either attempt. + let mut control = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + let resumed = control + .request("run.resume", json!({"run_id": run_id})) + .unwrap(); + assert_eq!(resumed["status"], "parked"); + let starts = journal_entries(&fixture.data_dir) + .unwrap() + .into_iter() + .filter(|entry| entry.entry_type == EntryType::StepAttemptStarted) + .count(); + assert_eq!(starts, 2, "live resume must preserve both original leases"); + + // Independent completions may arrive in reverse authored order. Once the + // earlier sibling finishes, the registry follows the remaining renewal. + let parked = complete(&mut worker, &second, json!({"answer": "a"})).unwrap(); + assert_eq!(parked["status"], "parked"); + assert_eq!( + registry.lookup(&run_id).unwrap().unwrap().next_wake_at_ms, + Some(renewed_deadline) + ); + let completed = complete(&mut worker, &first, json!({"answer": "b"})).unwrap(); + assert_eq!(completed["status"], "completed"); +} + +#[test] +fn server_restart_recovers_every_parallel_lease_without_duplicate_success() { + let fixture = LlmFixture::parallel("socket-crash-resume"); + let mut server = ServerGuard::start(&fixture); + let mut first_worker = attached_worker(&fixture, "before-crash"); + let run_id = start_run(&fixture); + let first_attempts = [ + first_worker.event("step.dispatch").unwrap(), + first_worker.event("step.dispatch").unwrap(), + ]; + server.kill(); + drop(first_worker); + + let _restarted = ServerGuard::start(&fixture); + let mut replacement = attached_worker(&fixture, "after-crash"); + let resume = spawn_resume(&fixture, &run_id); + let replacements = [ + replacement.event("step.dispatch").unwrap(), + replacement.event("step.dispatch").unwrap(), + ]; + assert_eq!(replacements[0]["attempt"], 2); + assert_eq!(replacements[1]["attempt"], 2); + + for original in &first_attempts { + let replacement_dispatch = replacements + .iter() + .find(|dispatch| dispatch["step_id"] == original["step_id"]) + .unwrap(); + assert_eq!( + replacement_dispatch["idempotency_key"], original["idempotency_key"], + "a resumed lane keeps its exactly-once effect key" + ); + } + + complete(&mut replacement, &replacements[0], json!({"answer": "b"})).unwrap(); + complete(&mut replacement, &replacements[1], json!({"answer": "a"})).unwrap(); + let output = resume.wait_with_output().unwrap(); + assert!(output.status.success(), "resume failed: {output:?}"); + + let entries = journal_entries(&fixture.data_dir).unwrap(); + for step_id in ["lane-b", "lane-a"] { + let completions = entries + .iter() + .filter(|entry| { + entry.entry_type == EntryType::StepCompleted + && entry.step_id.as_deref() == Some(step_id) + }) + .map(|entry| { + serde_json::from_value::(entry.payload.clone()).unwrap() + }) + .collect::>(); + assert_eq!( + completions + .iter() + .filter(|payload| payload.completion_reason == CompletionReason::Success) + .count(), + 1, + "each lane succeeds exactly once" + ); + assert!(completions.iter().any(|payload| matches!( + payload.completion_reason, + CompletionReason::Crashed | CompletionReason::LeaseExpired + ))); + } +} + /// Finding 1: two concurrent `run.resume` calls for the same runnable run must /// not both see the step Runnable — the scheduling decision is serialized per /// run, so exactly one attempt is journaled and dispatched. diff --git a/kernel/relayflowd/tests/crash_resume/llm_support.rs b/kernel/relayflowd/tests/crash_resume/llm_support.rs index 2d06c64d2..eef222529 100644 --- a/kernel/relayflowd/tests/crash_resume/llm_support.rs +++ b/kernel/relayflowd/tests/crash_resume/llm_support.rs @@ -4,6 +4,7 @@ use std::{ os::unix::{net::UnixStream, process::CommandExt}, path::{Path, PathBuf}, process::{Child, Command, Stdio}, + time::Duration, }; use anyhow::{Context, Result, bail}; @@ -90,6 +91,38 @@ impl LlmFixture { } } + pub fn parallel(name: &str) -> Self { + let mut fixture = Self::new(name, false); + fixture.spec = json!({ + "name": format!("llm-{name}"), + "steps": [ + { + "id": "lane-b", + "type": "llm", + "prompt": "research b", + "model": "deterministic-stub", + "max_iterations": 2, + "retry": {"initial_backoff_ms": 0, "max_backoff_ms": 0, "multiplier": 1, "jitter_percent": 0} + }, + { + "id": "lane-a", + "type": "llm", + "prompt": "research a", + "model": "deterministic-stub", + "max_iterations": 2, + "retry": {"initial_backoff_ms": 0, "max_backoff_ms": 0, "multiplier": 1, "jitter_percent": 0} + } + ], + "budget": {"max_tokens_in": 100, "max_tokens_out": 100, "max_dollars": "1"} + }); + fs::write( + &fixture.spec_path, + serde_json::to_vec(&fixture.spec).unwrap(), + ) + .unwrap(); + fixture + } + fn socket(&self) -> PathBuf { self.data_dir.join("relayflowd.sock") } @@ -195,6 +228,10 @@ impl ProtocolClient { } } + pub fn set_read_timeout(&self, timeout: Option) { + self.stream.set_read_timeout(timeout).unwrap(); + } + fn read_frame(&mut self) -> Result { let mut line = String::new(); if self.reader.read_line(&mut line)? == 0 { diff --git a/kernel/relayflowd/tests/parallel_driver.rs b/kernel/relayflowd/tests/parallel_driver.rs new file mode 100644 index 000000000..1e2790da6 --- /dev/null +++ b/kernel/relayflowd/tests/parallel_driver.rs @@ -0,0 +1,413 @@ +use std::{ + collections::BTreeSet, + fs, + os::unix::process::CommandExt, + panic::{AssertUnwindSafe, catch_unwind}, + process::Command, + sync::{ + Arc, Mutex, + atomic::{AtomicBool, Ordering}, + }, + time::{Duration, Instant}, +}; + +use relayflowd::worker::{DispatchOutcome, JournalObserver, StepDispatch, StepDispatcher}; +use relayflowd::{DriveOptions, Engine, OutOfBandCompletion, RunStatus}; +use relayflowd_core::{ + Budget, CompletionReason, EntryType, JournalEntry, RunSpec, StepCompletedPayload, StepType, +}; +use serde_json::json; +use tempfile::tempdir; + +#[derive(Default)] +struct StartCrashObserver { + crash_step: Option<&'static str>, + crashed: AtomicBool, + run_id: Mutex>, +} + +impl StartCrashObserver { + fn crashing_at(step_id: &'static str) -> Self { + Self { + crash_step: Some(step_id), + ..Self::default() + } + } + + fn run_id(&self) -> String { + self.run_id.lock().unwrap().clone().unwrap() + } +} + +impl JournalObserver for StartCrashObserver { + fn appended(&self, entry: &JournalEntry) { + if entry.entry_type == EntryType::RunSpawned { + *self.run_id.lock().unwrap() = Some(entry.run_id.clone()); + } + if entry.entry_type == EntryType::StepAttemptStarted + && entry.step_id.as_deref() == self.crash_step + && !self.crashed.swap(true, Ordering::SeqCst) + { + panic!("injected crash after durable start append"); + } + } +} + +#[derive(Default)] +struct RecordingDispatcher { + crash_step: Option<&'static str>, + mismatch_step: Option<&'static str>, + crash_fired: AtomicBool, + agent_available: bool, + calls: Mutex>, + effects: Mutex>, +} + +impl RecordingDispatcher { + fn llm() -> Self { + Self::default() + } + + fn crashing_at(step_id: &'static str) -> Self { + Self { + crash_step: Some(step_id), + ..Self::default() + } + } + + fn mismatching(step_id: &'static str) -> Self { + Self { + mismatch_step: Some(step_id), + ..Self::default() + } + } + + fn calls(&self) -> Vec { + self.calls.lock().unwrap().clone() + } +} + +impl StepDispatcher for RecordingDispatcher { + fn executor(&self, _step_type: StepType) -> Option { + Some("parallel-driver-test".to_owned()) + } + + fn available(&self, step_type: StepType) -> bool { + step_type == StepType::Llm || (step_type == StepType::Agent && self.agent_available) + } + + fn dispatch(&self, dispatch: StepDispatch) -> anyhow::Result { + self.calls.lock().unwrap().push(dispatch.clone()); + if self.mismatch_step == Some(dispatch.step_id.as_str()) { + return Ok(DispatchOutcome::PinMismatch { + detail: "injected replacement pin mismatch".to_owned(), + }); + } + self.effects + .lock() + .unwrap() + .insert((dispatch.step_id.clone(), dispatch.idempotency_key.clone())); + if self.crash_step == Some(dispatch.step_id.as_str()) + && !self.crash_fired.swap(true, Ordering::SeqCst) + { + panic!("injected crash after worker handoff"); + } + Ok(DispatchOutcome::Dispatched) + } +} + +fn parallel_llm_spec() -> RunSpec { + RunSpec::parse(&json!({ + "name": "parallel-driver", + "steps": [ + { + "id": "lane-b", + "type": "llm", + "prompt": "b", + "model": "stub", + "max_iterations": 2, + "retry": {"initial_backoff_ms": 0, "max_backoff_ms": 0, "multiplier": 1, "jitter_percent": 0} + }, + { + "id": "lane-a", + "type": "llm", + "prompt": "a", + "model": "stub", + "max_iterations": 2, + "retry": {"initial_backoff_ms": 0, "max_backoff_ms": 0, "multiplier": 1, "jitter_percent": 0} + } + ] + })) + .unwrap() +} + +fn complete(engine: &Engine, run_id: &str, dispatch: &StepDispatch) -> RunStatus { + engine + .complete_out_of_band( + run_id, + &dispatch.step_id, + OutOfBandCompletion { + attempt: dispatch.attempt, + idempotency_key: dispatch.idempotency_key.clone(), + completion_reason: CompletionReason::Success, + output: json!({"answer": dispatch.step_id}), + budget: Budget::default(), + completed_by: "parallel-driver-test".to_owned(), + started_pins: None, + end_pins: None, + effects: Vec::new(), + trajectory_tail: None, + }, + ) + .unwrap() + .status +} + +#[test] +fn crash_boundaries_resume_the_real_driver_with_one_effect_per_lane() { + enum Boundary { + FirstStart, + FirstDispatch, + AllStarts, + } + + for boundary in [ + Boundary::FirstStart, + Boundary::FirstDispatch, + Boundary::AllStarts, + ] { + let directory = tempdir().unwrap(); + let observer = Arc::new(match boundary { + Boundary::FirstStart => StartCrashObserver::crashing_at("lane-b"), + Boundary::AllStarts => StartCrashObserver::crashing_at("lane-a"), + Boundary::FirstDispatch => StartCrashObserver::default(), + }); + let dispatcher = Arc::new(match boundary { + Boundary::FirstDispatch => RecordingDispatcher::crashing_at("lane-b"), + _ => RecordingDispatcher::llm(), + }); + let engine = Engine::with_runtime(directory.path(), dispatcher.clone(), observer.clone()); + + let crashed = catch_unwind(AssertUnwindSafe(|| { + engine.start(parallel_llm_spec(), "test", None) + })); + assert!(crashed.is_err(), "the selected driver boundary must crash"); + let run_id = observer.run_id(); + + let resumed = engine.resume(&run_id, None).unwrap(); + assert_eq!(resumed.status, RunStatus::Parked); + let calls = dispatcher.calls(); + let active = ["lane-a", "lane-b"].map(|step_id| { + calls + .iter() + .filter(|dispatch| dispatch.step_id == step_id) + .max_by_key(|dispatch| dispatch.attempt) + .unwrap() + .clone() + }); + + // Complete in reverse authored order; the barrier is the journaled + // state, not the order in which independent workers return. + assert_eq!(complete(&engine, &run_id, &active[0]), RunStatus::Parked); + assert_eq!(complete(&engine, &run_id, &active[1]), RunStatus::Completed); + + let effects = dispatcher.effects.lock().unwrap(); + assert_eq!(effects.len(), 2, "one deduplicated effect per lane"); + for step_id in ["lane-b", "lane-a"] { + assert_eq!( + effects.iter().filter(|(step, _)| step == step_id).count(), + 1, + "{step_id} must keep one effect identity across recovery" + ); + let keys = calls + .iter() + .filter(|dispatch| dispatch.step_id == step_id) + .map(|dispatch| dispatch.idempotency_key.as_str()) + .collect::>(); + assert_eq!(keys.len(), 1, "{step_id} idempotency key changed"); + } + drop(effects); + + let entries = engine.journal_entries(&run_id, 1, usize::MAX).unwrap(); + for step_id in ["lane-b", "lane-a"] { + let successes = entries + .iter() + .filter(|entry| { + entry.entry_type == EntryType::StepCompleted + && entry.step_id.as_deref() == Some(step_id) + }) + .filter(|entry| { + serde_json::from_value::(entry.payload.clone()) + .unwrap() + .completion_reason + == CompletionReason::Success + }) + .count(); + assert_eq!(successes, 1, "{step_id} must succeed exactly once"); + } + } +} + +#[test] +fn backpressured_or_mismatched_lane_does_not_drop_a_later_dispatch() { + let mixed_spec = RunSpec::parse(&json!({ + "name": "mixed-backpressure", + "steps": [ + { + "id": "agent-unavailable", + "type": "agent", + "instruction": "edit", + "recovery_mode": "inspect", + "surfaces": {"workspace": [], "streams": [], "external": []} + }, + {"id": "llm-ready", "type": "llm", "prompt": "ready", "model": "stub"} + ] + })) + .unwrap(); + let directory = tempdir().unwrap(); + let dispatcher = Arc::new(RecordingDispatcher::llm()); + let observer = Arc::new(StartCrashObserver::default()); + let engine = Engine::with_runtime(directory.path(), dispatcher.clone(), observer.clone()); + assert_eq!( + engine.start(mixed_spec, "test", None).unwrap().status, + RunStatus::Parked + ); + assert_eq!( + dispatcher + .calls() + .iter() + .map(|dispatch| dispatch.step_id.as_str()) + .collect::>(), + ["llm-ready"] + ); + let entries = engine + .journal_entries(&observer.run_id(), 1, usize::MAX) + .unwrap(); + assert!(!entries.iter().any(|entry| { + entry.entry_type == EntryType::StepAttemptStarted + && entry.step_id.as_deref() == Some("agent-unavailable") + })); + + let directory = tempdir().unwrap(); + let dispatcher = Arc::new(RecordingDispatcher::mismatching("lane-b")); + let observer = Arc::new(StartCrashObserver::default()); + let engine = Engine::with_runtime(directory.path(), dispatcher.clone(), observer); + engine.start(parallel_llm_spec(), "test", None).unwrap(); + assert_eq!( + dispatcher + .calls() + .iter() + .map(|dispatch| dispatch.step_id.as_str()) + .collect::>(), + ["lane-b", "lane-a"], + "a mismatched first lane must not discard the later dispatch" + ); +} + +#[test] +fn stop_after_one_holds_for_an_independent_deterministic_batch() { + let spec = RunSpec::parse(&json!({ + "name": "deterministic-stop", + "steps": [ + {"id": "first", "type": "deterministic", "command": "true"}, + {"id": "second", "type": "deterministic", "command": "true"} + ] + })) + .unwrap(); + let directory = tempdir().unwrap(); + let engine = Engine::new(directory.path()); + let outcome = engine + .start_with_options( + spec, + "test", + DriveOptions { + stop_after: Some(1), + ..DriveOptions::default() + }, + ) + .unwrap(); + assert_eq!(outcome.status, RunStatus::Interrupted); + assert_eq!(outcome.completed_steps, 1); + let entries = engine + .journal_entries(&outcome.run_id, 1, usize::MAX) + .unwrap(); + assert!(!entries.iter().any(|entry| { + entry.entry_type == EntryType::StepAttemptStarted + && entry.step_id.as_deref() == Some("second") + })); +} + +#[test] +fn pause_before_second_independent_step_holds_the_driver_boundary() { + let directory = tempdir().unwrap(); + let data_dir = directory.path().join("data"); + let marker = directory.path().join("effects.txt"); + let spec_path = directory.path().join("parallel.json"); + let spec = json!({ + "name": "deterministic-pause", + "steps": [ + { + "id": "first", + "type": "deterministic", + "command": ["/bin/sh", "-c", format!("printf 'first\\n' >> '{}'", marker.display())] + }, + { + "id": "second", + "type": "deterministic", + "command": ["/bin/sh", "-c", format!("printf 'second\\n' >> '{}'", marker.display())] + } + ] + }); + fs::write(&spec_path, serde_json::to_vec(&spec).unwrap()).unwrap(); + let mut child = Command::new(env!("CARGO_BIN_EXE_relayflowd")) + .args([ + "--data-dir", + data_dir.to_str().unwrap(), + "run", + spec_path.to_str().unwrap(), + "--pause-before-step", + "second", + ]) + .process_group(0) + .spawn() + .unwrap(); + + let deadline = Instant::now() + Duration::from_secs(15); + while fs::read_to_string(&marker).unwrap_or_default() != "first\n" { + assert!( + Instant::now() < deadline, + "driver never reached second lane" + ); + std::thread::sleep(Duration::from_millis(20)); + } + let run_id = fs::read_dir(data_dir.join("runs")) + .unwrap() + .next() + .unwrap() + .unwrap() + .path() + .file_stem() + .unwrap() + .to_str() + .unwrap() + .to_owned(); + let entries = Engine::new(&data_dir) + .journal_entries(&run_id, 1, usize::MAX) + .unwrap(); + assert!(!entries.iter().any(|entry| { + entry.entry_type == EntryType::StepAttemptStarted + && entry.step_id.as_deref() == Some("second") + })); + + // SAFETY: kill(2) with a negative process-group id does not access memory. + assert_eq!( + unsafe { libc::kill(-(child.id() as i32), libc::SIGKILL) }, + 0 + ); + assert!(!child.wait().unwrap().success()); + assert_eq!( + Engine::new(&data_dir).resume(&run_id, None).unwrap().status, + RunStatus::Completed + ); + assert_eq!(fs::read_to_string(marker).unwrap(), "first\nsecond\n"); +} From 382c5c1eb8c3042f760a378fcab41702161b47bb Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 20:02:00 +0200 Subject: [PATCH 03/14] fix(kernel): preserve parallel assignment lifecycle Session-Id: 01a062cc-f525-7d01-932e-a634815114c1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine.rs | 21 +- .../relayflowd-core/src/machine/parallel.rs | 70 ++++ .../src/machine/parallel_tests.rs | 233 ++++++++++++- kernel/relayflowd-core/src/state.rs | 5 + kernel/relayflowd/src/engine.rs | 18 +- kernel/relayflowd/src/engine/drive.rs | 28 +- kernel/relayflowd/src/server/session.rs | 9 + kernel/relayflowd/src/worker.rs | 7 + kernel/relayflowd/tests/crash_resume.rs | 2 + .../tests/crash_resume/llm_support.rs | 53 +++ .../tests/crash_resume/parallel_lifecycle.rs | 319 ++++++++++++++++++ kernel/relayflowd/tests/parallel_driver.rs | 68 +++- 12 files changed, 812 insertions(+), 21 deletions(-) create mode 100644 kernel/relayflowd-core/src/machine/parallel.rs create mode 100644 kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs diff --git a/kernel/relayflowd-core/src/machine.rs b/kernel/relayflowd-core/src/machine.rs index 7ff694524..b2924ae14 100644 --- a/kernel/relayflowd-core/src/machine.rs +++ b/kernel/relayflowd-core/src/machine.rs @@ -95,6 +95,15 @@ pub fn next_actions(state: &RunState, now_ms: i64) -> Vec { return cancel_run_actions(state, now_ms); } if let Some(failed_step_id) = state.failed_step() { + if state + .steps + .values() + .any(|runtime| matches!(runtime.state, StepState::Running { .. })) + { + // Drain already-started siblings before making run.completed + // terminal. No fresh work is elected once failure is inevitable. + return Vec::new(); + } return complete_run_actions( state, RunCompletionReason::StepFailed, @@ -147,14 +156,8 @@ pub fn next_actions(state: &RunState, now_ms: i64) -> Vec { // authored spec order while emitting every journal-first start pair from // that one state snapshot; dependencies unlocked by these executions are // considered only after their completions are folded on the next pass. - let starts = state - .spec - .steps - .iter() - .filter_map(|spec| { - let runtime = &state.steps[&spec.id]; - (runtime.state == StepState::Runnable).then_some((spec, runtime)) - }) + let starts = parallel::runnable_batch(state) + .into_iter() .flat_map(|(spec, runtime)| start_actions(state, spec, runtime.attempts + 1, now_ms)) .collect::>(); if !starts.is_empty() { @@ -457,6 +460,8 @@ fn deterministic_ulid( mod recovery; pub use recovery::{abandonment_actions, recovery_actions, recovery_actions_filtered}; +mod parallel; + #[cfg(test)] mod parallel_tests; #[cfg(test)] diff --git a/kernel/relayflowd-core/src/machine/parallel.rs b/kernel/relayflowd-core/src/machine/parallel.rs new file mode 100644 index 000000000..335b9c98c --- /dev/null +++ b/kernel/relayflowd-core/src/machine/parallel.rs @@ -0,0 +1,70 @@ +//! Deterministic parallel batch selection. +//! +//! DAG independence is not enough for agent work: two steps can be topology- +//! independent while mutating the same declared surface. The authored-order +//! maximal batch therefore reserves each mutable surface once. An unfinished +//! attempt in backoff or waiting retains its reservation just like a live +//! lease, preventing a crash/retry from turning into a last-write-wins fork. + +use std::collections::BTreeSet; + +use crate::{ + spec::{StepKind, StepSpec}, + state::{RunState, StepRuntime, StepState}, +}; + +pub(super) fn runnable_batch(state: &RunState) -> Vec<(&StepSpec, &StepRuntime)> { + let mut occupied = state + .spec + .steps + .iter() + .filter(|step| { + matches!( + state.steps[&step.id].state, + StepState::Running { .. } + | StepState::Backoff { .. } + | StepState::Waiting { .. } + | StepState::NeedsHuman { .. } + ) + }) + .flat_map(surface_keys) + .collect::>(); + let mut selected = Vec::new(); + for step in &state.spec.steps { + let runtime = &state.steps[&step.id]; + if runtime.state != StepState::Runnable { + continue; + } + let surfaces = surface_keys(step).collect::>(); + if surfaces.iter().any(|surface| occupied.contains(surface)) { + continue; + } + occupied.extend(surfaces); + selected.push((step, runtime)); + } + selected +} + +fn surface_keys(step: &StepSpec) -> impl Iterator + '_ { + let StepKind::Agent { surfaces, .. } = &step.kind else { + return Vec::new().into_iter(); + }; + surfaces + .workspace + .iter() + .map(|surface| format!("workspace:{}", surface.surface)) + .chain( + surfaces + .streams + .iter() + .map(|surface| format!("stream:{}", surface.stream)), + ) + .chain( + surfaces + .external + .iter() + .map(|surface| format!("external:{surface}")), + ) + .collect::>() + .into_iter() +} diff --git a/kernel/relayflowd-core/src/machine/parallel_tests.rs b/kernel/relayflowd-core/src/machine/parallel_tests.rs index 8340d1839..769319754 100644 --- a/kernel/relayflowd-core/src/machine/parallel_tests.rs +++ b/kernel/relayflowd-core/src/machine/parallel_tests.rs @@ -1,7 +1,10 @@ use serde_json::json; use super::*; -use crate::{entry::StepCompletedPayload, state::RunState}; +use crate::{ + entry::{StepCompletedPayload, WorkspacePin}, + state::{RunState, StateError}, +}; fn parallel_spec() -> crate::RunSpec { crate::RunSpec::parse(&json!({ @@ -39,6 +42,65 @@ fn appended_entries(actions: &[Action]) -> Vec { .collect() } +fn parallel_agent_spec(overlapping: bool) -> crate::RunSpec { + let lane_a_surface = if overlapping { "repo-b" } else { "repo-a" }; + crate::RunSpec::parse(&json!({ + "steps": [ + { + "id": "lane-b", + "type": "agent", + "instruction": "b", + "surfaces": {"workspace": [{"surface": "repo-b"}]} + }, + { + "id": "lane-a", + "type": "agent", + "instruction": "a", + "surfaces": {"workspace": [{"surface": lane_a_surface}]} + }, + { + "id": "join", + "type": "agent", + "instruction": "join", + "depends_on": ["lane-b", "lane-a"], + "surfaces": {"workspace": [ + {"surface": "repo-b"}, + {"surface": lane_a_surface} + ]} + } + ] + })) + .unwrap() +} + +fn agent_success( + spec: &crate::RunSpec, + step_id: &str, + revision: &str, + now_ms: i64, +) -> JournalEntry { + let step = spec.step(step_id).unwrap(); + let mut result = AttemptResult::successful(json!({"done": step_id}), "worker"); + let surface = match &step.kind { + StepKind::Agent { surfaces, .. } => surfaces.workspace[0].surface.clone(), + _ => unreachable!(), + }; + result.end_pins = Some(Pins { + workspace: vec![WorkspacePin { + surface, + revision_id: revision.to_owned(), + }], + streams: Vec::new(), + }); + completion_actions("run", step, 1, 0, result, now_ms) + .into_iter() + .find_map(|action| match action { + Action::Append(entry) if entry.entry_type == EntryType::StepCompleted => Some(entry), + _ => None, + }) + .unwrap() +} + #[test] fn machine_starts_every_runnable_step_in_authored_order() { let state = RunState::fold("run", parallel_spec(), &[]).unwrap(); @@ -165,3 +227,172 @@ fn crash_resume_preserves_each_parallel_lease_exactly_once() { assert_eq!(started.attempt, Some(2)); } } + +#[test] +fn overlapping_agent_surfaces_are_serialized_in_authored_order() { + let spec = parallel_agent_spec(true); + let fresh = RunState::fold("run", spec.clone(), &[]).unwrap(); + let first = next_actions(&fresh, 10); + assert_eq!(first.len(), 2, "only the authored conflict winner starts"); + let Action::Append(started) = &first[0] else { + panic!("the winning agent must journal its start") + }; + assert_eq!(started.step_id.as_deref(), Some("lane-b")); + + let mut entries = appended_entries(&first); + let running = RunState::fold("run", spec.clone(), &entries).unwrap(); + assert!( + next_actions(&running, 11).is_empty(), + "a conflicting lane must remain runnable while the surface is leased" + ); + let mut recovering_entries = entries.clone(); + recovering_entries.extend(appended_entries(&recovery_actions(&running, 12))); + let recovering = RunState::fold("run", spec.clone(), &recovering_entries).unwrap(); + assert!( + next_actions(&recovering, 12) + .iter() + .all(|action| matches!(action, Action::ArmTimer { .. })), + "the conflicting sibling must not pass an unfinished lane in retry backoff" + ); + + entries.push(agent_success(&spec, "lane-b", "rB", 12)); + let released = RunState::fold("run", spec, &entries).unwrap(); + let second = next_actions(&released, 12); + let Action::Append(started) = &second[0] else { + panic!("the released conflicting lane must now start") + }; + assert_eq!(started.step_id.as_deref(), Some("lane-a")); + let payload: AttemptStartedPayload = serde_json::from_value(started.payload.clone()).unwrap(); + assert_eq!(payload.pins.workspace[0].revision_id, "rB"); +} + +#[test] +fn every_declared_mutable_surface_participates_in_conflict_selection() { + for surfaces in [ + json!({"workspace": [{"surface": "repo"}]}), + json!({"streams": [{"stream": "notes"}]}), + json!({"external": ["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/provider/item"]}), + ] { + let spec = crate::RunSpec::parse(&json!({ + "steps": [ + {"id": "first", "type": "agent", "instruction": "a", "surfaces": surfaces}, + {"id": "second", "type": "agent", "instruction": "b", "surfaces": surfaces} + ] + })) + .unwrap(); + let state = RunState::fold("run", spec, &[]).unwrap(); + let actions = next_actions(&state, 10); + assert_eq!(actions.len(), 2); + let Action::Append(started) = &actions[0] else { + panic!("the authored conflict winner must start") + }; + assert_eq!(started.step_id.as_deref(), Some("first")); + } +} + +#[test] +fn disjoint_agent_lanes_merge_pins_in_either_completion_order() { + for order in [["lane-b", "lane-a"], ["lane-a", "lane-b"]] { + let spec = parallel_agent_spec(false); + let fresh = RunState::fold("run", spec.clone(), &[]).unwrap(); + let starts = next_actions(&fresh, 10); + assert_eq!(starts.len(), 4, "disjoint agent lanes may fan out"); + let mut entries = appended_entries(&starts); + for step_id in order { + let revision = if step_id == "lane-b" { "rB" } else { "rA" }; + entries.push(agent_success(&spec, step_id, revision, 20)); + } + let joined = RunState::fold("run", spec, &entries).unwrap(); + let actions = next_actions(&joined, 20); + let Action::Append(started) = &actions[0] else { + panic!("the join must start after both disjoint lanes") + }; + let payload: AttemptStartedPayload = + serde_json::from_value(started.payload.clone()).unwrap(); + assert!( + payload + .pins + .workspace + .iter() + .any(|pin| { pin.surface == "repo-b" && pin.revision_id == "rB" }) + ); + assert!( + payload + .pins + .workspace + .iter() + .any(|pin| { pin.surface == "repo-a" && pin.revision_id == "rA" }) + ); + } +} + +#[test] +fn failed_run_drains_open_siblings_before_terminal_entry() { + let spec = parallel_spec(); + let fresh = RunState::fold("run", spec.clone(), &[]).unwrap(); + let mut entries = appended_entries(&next_actions(&fresh, 10)); + let mut failed = AttemptResult::successful(Value::Null, "worker"); + failed.failure_reason = Some(CompletionReason::WorkerError); + entries.extend(appended_entries(&completion_actions( + "run", + &spec.steps[0], + 1, + 0, + failed, + 20, + ))); + let draining = RunState::fold("run", spec.clone(), &entries).unwrap(); + assert!( + next_actions(&draining, 20).is_empty(), + "run.completed must wait for the open sibling lease" + ); + + entries.push( + completion_actions( + "run", + &spec.steps[2], + 1, + 0, + AttemptResult::successful(json!({"answer": "a"}), "worker"), + 21, + ) + .into_iter() + .find_map(|action| match action { + Action::Append(entry) if entry.entry_type == EntryType::StepCompleted => Some(entry), + _ => None, + }) + .unwrap(), + ); + let drained = RunState::fold("run", spec.clone(), &entries).unwrap(); + let terminal = next_actions(&drained, 21); + assert!(matches!( + &terminal[..], + [Action::Append(entry), Action::CompleteRun { .. }] + if entry.entry_type == EntryType::RunCompleted + )); + + entries.extend(appended_entries(&terminal)); + entries.push(JournalEntry::new( + EntryType::StepCompleted, + "run", + Some("lane-a".to_owned()), + Some(1), + 22, + StepCompletedPayload { + completion_reason: CompletionReason::Success, + disposition: Disposition::StepDone, + output: Value::Null, + verification: None, + end_pins: None, + effects: Vec::new(), + trajectory_tail: None, + budget: Budget::default(), + completed_by: "late-worker".to_owned(), + next_attempt_at_ms: None, + }, + )); + assert!(matches!( + RunState::fold("run", spec, &entries), + Err(StateError::EntryAfterRunCompleted { .. }) + )); +} diff --git a/kernel/relayflowd-core/src/state.rs b/kernel/relayflowd-core/src/state.rs index 3409909f6..ea97596a9 100644 --- a/kernel/relayflowd-core/src/state.rs +++ b/kernel/relayflowd-core/src/state.rs @@ -114,6 +114,9 @@ impl RunState { if entry.run_id != state.run_id { return Err(StateError::WrongRun(entry.run_id.clone())); } + if state.completion.is_some() { + return Err(StateError::EntryAfterRunCompleted { seq: entry.seq }); + } match entry.entry_type { EntryType::EpochSummary => state.apply_epoch(entry)?, EntryType::StepAttemptStarted => { @@ -412,6 +415,8 @@ pub enum StateError { MissingAttempt(i64), #[error("journal references unknown step {0}")] UnknownStep(String), + #[error("journal entry {seq} appears after terminal run.completed")] + EntryAfterRunCompleted { seq: i64 }, #[error("agent step {0} completed successfully without end pins")] MissingEndPins(String), #[error( diff --git a/kernel/relayflowd/src/engine.rs b/kernel/relayflowd/src/engine.rs index 7484235ae..a9ee9ae28 100644 --- a/kernel/relayflowd/src/engine.rs +++ b/kernel/relayflowd/src/engine.rs @@ -182,13 +182,17 @@ impl Engine { let spec = journal.run_spec().context("read run spec")?; let state = self.load_state(&journal, spec)?; let mut snapshot = snapshot_from_state(&state); - let registry_record = self.registry()?.lookup(run_id)?; - if let Some(record) = registry_record.filter(|record| record.status == "waiting_worker") { - for step in snapshot.steps.values_mut().filter(|step| { - step.step_type != relayflowd_core::StepType::Deterministic - && step.state == model::StepStatus::Running - }) { - step.lease_deadline_ms = record.next_wake_at_ms; + if let Some(dispatcher) = &self.dispatcher { + for step in &state.spec.steps { + let relayflowd_core::StepState::Running { attempt, .. } = + state.steps[&step.id].state + else { + continue; + }; + if let Some(deadline) = dispatcher.active_lease_deadline(run_id, &step.id, attempt) + { + snapshot.steps.get_mut(&step.id).unwrap().lease_deadline_ms = Some(deadline); + } } } Ok(snapshot) diff --git a/kernel/relayflowd/src/engine/drive.rs b/kernel/relayflowd/src/engine/drive.rs index d02c6d3cd..f77802c74 100644 --- a/kernel/relayflowd/src/engine/drive.rs +++ b/kernel/relayflowd/src/engine/drive.rs @@ -22,6 +22,7 @@ impl Engine { ) -> Result { let initial_completed = self.load_state(&journal, spec.clone())?.completed_steps(); let mut pause_consumed = false; + let mut failed_batch_redriven = false; loop { let state = self.load_state(&journal, spec.clone())?; if let Some(reason) = state.completion { @@ -46,6 +47,7 @@ impl Engine { } let mut dispatched = false; let mut backpressured = false; + let mut handoff_failed = false; let mut skipped_dispatches = BTreeSet::new(); for action in actions { match action { @@ -179,6 +181,7 @@ impl Engine { self.persist_only(&mut journal, action)?; } backpressured = true; + handoff_failed = true; } DispatchOutcome::PinMismatch { detail } => { // Appendix A rule 2: a worker standing at @@ -192,6 +195,7 @@ impl Engine { attempt, detail, )?; + handoff_failed = true; } } } @@ -214,6 +218,15 @@ impl Engine { } } } + if !dispatched && handoff_failed && !failed_batch_redriven { + // A whole batch can lose admission between preflight and + // handoff. Re-fold once so due retries can reach a compatible + // replacement already attached. Bound this to one failed + // batch: a dispatcher that keeps racing closed must park + // instead of spinning forever. + failed_batch_redriven = true; + continue; + } if dispatched || backpressured { let parked = self.load_state(&journal, spec.clone())?; self.park_idle_run(&parked)?; @@ -310,10 +323,17 @@ impl Engine { .iter() .filter_map(|step| match state.steps[&step.id].state { relayflowd_core::StepState::Running { - lease_deadline_ms, .. - } if step.step_type() != relayflowd_core::StepType::Deterministic => { - Some(lease_deadline_ms) - } + attempt, + lease_deadline_ms, + .. + } if step.step_type() != relayflowd_core::StepType::Deterministic => Some( + self.dispatcher + .as_ref() + .and_then(|dispatcher| { + dispatcher.active_lease_deadline(&state.run_id, &step.id, attempt) + }) + .unwrap_or(lease_deadline_ms), + ), _ => None, }) .min(); diff --git a/kernel/relayflowd/src/server/session.rs b/kernel/relayflowd/src/server/session.rs index 6bbff6e80..bac789af8 100644 --- a/kernel/relayflowd/src/server/session.rs +++ b/kernel/relayflowd/src/server/session.rs @@ -459,6 +459,15 @@ impl StepDispatcher for ProtocolHub { ); Ok(DispatchOutcome::Dispatched) } + + fn active_lease_deadline(&self, run_id: &str, step_id: &str, attempt: u32) -> Option { + self.sessions + .lock() + .expect("protocol sessions lock") + .assignments + .get(&(run_id.to_owned(), step_id.to_owned(), attempt)) + .map(|assignment| assignment.lease_deadline_ms) + } } impl LeaseProbe for ProtocolHub { diff --git a/kernel/relayflowd/src/worker.rs b/kernel/relayflowd/src/worker.rs index af119b26f..8a8bf19d4 100644 --- a/kernel/relayflowd/src/worker.rs +++ b/kernel/relayflowd/src/worker.rs @@ -51,6 +51,13 @@ pub trait StepDispatcher: Send + Sync { } fn dispatch(&self, dispatch: StepDispatch) -> Result; + + /// Heartbeat-renewed operational deadline for one live assignment. The + /// journal retains the original grant; a live server projection must use + /// the assignment it currently owns instead of rewriting that history. + fn active_lease_deadline(&self, _run_id: &str, _step_id: &str, _attempt: u32) -> Option { + None + } } /// Journal watches are projections. Notification happens only after append. diff --git a/kernel/relayflowd/tests/crash_resume.rs b/kernel/relayflowd/tests/crash_resume.rs index e956b1ad0..51e5b6014 100644 --- a/kernel/relayflowd/tests/crash_resume.rs +++ b/kernel/relayflowd/tests/crash_resume.rs @@ -11,6 +11,8 @@ mod concurrency; mod llm; #[path = "crash_resume/llm_support.rs"] mod llm_support; +#[path = "crash_resume/parallel_lifecycle.rs"] +mod parallel_lifecycle; #[path = "crash_resume/support.rs"] mod support; diff --git a/kernel/relayflowd/tests/crash_resume/llm_support.rs b/kernel/relayflowd/tests/crash_resume/llm_support.rs index eef222529..1fa104bfc 100644 --- a/kernel/relayflowd/tests/crash_resume/llm_support.rs +++ b/kernel/relayflowd/tests/crash_resume/llm_support.rs @@ -123,6 +123,59 @@ impl LlmFixture { fixture } + pub fn parallel_terminal(name: &str) -> Self { + let mut fixture = Self::parallel(name); + for step in fixture.spec["steps"].as_array_mut().unwrap() { + step["max_iterations"] = json!(1); + } + fs::write( + &fixture.spec_path, + serde_json::to_vec(&fixture.spec).unwrap(), + ) + .unwrap(); + fixture + } + + pub fn parallel_agents(name: &str, overlapping: bool) -> Self { + let mut fixture = Self::new(name, false); + let lane_a_surface = if overlapping { "repo-b" } else { "repo-a" }; + let join_workspace = if overlapping { + json!([{"surface": "repo-b"}]) + } else { + json!([{"surface": "repo-b"}, {"surface": "repo-a"}]) + }; + fixture.spec = json!({ + "name": format!("agent-parallel-{name}"), + "steps": [ + { + "id": "lane-b", + "type": "agent", + "instruction": "b", + "surfaces": {"workspace": [{"surface": "repo-b"}]} + }, + { + "id": "lane-a", + "type": "agent", + "instruction": "a", + "surfaces": {"workspace": [{"surface": lane_a_surface}]} + }, + { + "id": "join", + "type": "agent", + "instruction": "join", + "depends_on": ["lane-b", "lane-a"], + "surfaces": {"workspace": join_workspace} + } + ] + }); + fs::write( + &fixture.spec_path, + serde_json::to_vec(&fixture.spec).unwrap(), + ) + .unwrap(); + fixture + } + fn socket(&self) -> PathBuf { self.data_dir.join("relayflowd.sock") } diff --git a/kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs b/kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs new file mode 100644 index 000000000..702147e82 --- /dev/null +++ b/kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs @@ -0,0 +1,319 @@ +//! Multi-lease lifecycle tests at the real Unix-socket and CLI boundary. + +use std::{ + thread, + time::{Duration, Instant}, +}; + +use relayflowd_core::EntryType; +use serde_json::{Value, json}; + +use super::{ + llm_support::{ + LlmFixture, ProtocolClient, ServerGuard, attached_worker, complete, spawn_resume, start_run, + }, + support::{journal_entries, wait_until}, +}; + +fn attached_agent(fixture: &LlmFixture, id: &str) -> ProtocolClient { + let mut worker = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + worker + .request( + "worker.attach", + json!({ + "worker_id": id, + "step_types": ["agent"], + "pins": {"workspace": [ + {"surface": "repo-b", "revision_id": "r0"}, + {"surface": "repo-a", "revision_id": "r0"} + ]} + }), + ) + .unwrap(); + worker +} + +fn complete_agent( + worker: &mut ProtocolClient, + dispatch: &Value, + revisions: &[(&str, &str)], +) -> anyhow::Result { + worker.request( + "step.complete", + json!({ + "run_id": dispatch["run_id"], + "step_id": dispatch["step_id"], + "attempt": dispatch["attempt"], + "idempotency_key": dispatch["idempotency_key"], + "completionReason": "success", + "output": {"done": dispatch["step_id"]}, + "started_pins": dispatch["pins"], + "end_pins": {"workspace": revisions.iter().map(|(surface, revision_id)| { + json!({"surface": surface, "revision_id": revision_id}) + }).collect::>()} + }), + ) +} + +fn complete_failure(worker: &mut ProtocolClient, dispatch: &Value) -> anyhow::Result { + worker.request( + "step.complete", + json!({ + "run_id": dispatch["run_id"], + "step_id": dispatch["step_id"], + "attempt": dispatch["attempt"], + "idempotency_key": dispatch["idempotency_key"], + "completionReason": "worker_error", + "output": null + }), + ) +} + +#[test] +fn renewed_parallel_leases_survive_the_original_grant_and_remain_distinct() { + let fixture = LlmFixture::parallel("renewed-cli-resume"); + let _server = ServerGuard::start(&fixture); + let mut worker = attached_worker(&fixture, "renewed-stub"); + let run_id = start_run(&fixture); + let dispatches = [ + worker.event("step.dispatch").unwrap(), + worker.event("step.dispatch").unwrap(), + ]; + let began = Instant::now(); + thread::sleep(Duration::from_secs(10)); + let renew = |worker: &mut ProtocolClient, dispatch: &Value| { + worker + .request( + "step.heartbeat", + json!({ + "run_id": run_id, + "step_id": dispatch["step_id"], + "attempt": dispatch["attempt"], + "lease_id": dispatch["lease_id"] + }), + ) + .unwrap()["lease_deadline_ms"] + .as_i64() + .unwrap() + }; + let lane_b_deadline = renew(&mut worker, &dispatches[0]); + thread::sleep(Duration::from_millis(40)); + let lane_a_deadline = renew(&mut worker, &dispatches[1]); + assert_ne!(lane_b_deadline, lane_a_deadline); + let mut control = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + let snapshot = control + .request("run.get", json!({"run_id": run_id})) + .unwrap(); + assert_eq!( + snapshot["steps"]["lane-b"]["lease_deadline_ms"], + lane_b_deadline + ); + assert_eq!( + snapshot["steps"]["lane-a"]["lease_deadline_ms"], + lane_a_deadline + ); + + thread::sleep(Duration::from_secs(36).saturating_sub(began.elapsed())); + let mut resume = spawn_resume(&fixture, &run_id); + thread::sleep(Duration::from_millis(300)); + assert!( + resume.try_wait().unwrap().is_none(), + "the real CLI must keep waiting on renewed assignments" + ); + complete(&mut worker, &dispatches[1], json!({"answer": "a"})).unwrap(); + complete(&mut worker, &dispatches[0], json!({"answer": "b"})).unwrap(); + let output = resume.wait_with_output().unwrap(); + assert!(output.status.success(), "renewed resume failed: {output:?}"); +} + +#[test] +fn overlapping_agent_lanes_serialize_while_disjoint_lanes_merge_in_either_order() { + let fixture = LlmFixture::parallel_agents("overlap", true); + let _server = ServerGuard::start(&fixture); + let mut worker = attached_agent(&fixture, "overlap-agent"); + start_run(&fixture); + let lane_b = worker.event("step.dispatch").unwrap(); + assert_eq!(lane_b["step_id"], "lane-b"); + worker.set_read_timeout(Some(Duration::from_millis(200))); + assert!(worker.event("step.dispatch").is_err()); + worker.set_read_timeout(None); + complete_agent(&mut worker, &lane_b, &[("repo-b", "rB")]).unwrap(); + let lane_a = worker.event("step.dispatch").unwrap(); + assert_eq!(lane_a["pins"]["workspace"][0]["revision_id"], "rB"); + complete_agent(&mut worker, &lane_a, &[("repo-b", "rA")]).unwrap(); + let join = worker.event("step.dispatch").unwrap(); + assert_eq!(join["pins"]["workspace"][0]["revision_id"], "rA"); + assert_eq!( + complete_agent(&mut worker, &join, &[("repo-b", "rJ")]).unwrap()["status"], + "completed" + ); + + for (name, order) in [ + ("disjoint-authored", ["lane-b", "lane-a"]), + ("disjoint-reverse", ["lane-a", "lane-b"]), + ] { + let fixture = LlmFixture::parallel_agents(name, false); + let _server = ServerGuard::start(&fixture); + let mut worker = attached_agent(&fixture, name); + start_run(&fixture); + let dispatches = [ + worker.event("step.dispatch").unwrap(), + worker.event("step.dispatch").unwrap(), + ]; + for step_id in order { + let dispatch = dispatches + .iter() + .find(|dispatch| dispatch["step_id"] == step_id) + .unwrap(); + let pins = if step_id == "lane-b" { + &[("repo-b", "rB")][..] + } else { + &[("repo-a", "rA")][..] + }; + complete_agent(&mut worker, dispatch, pins).unwrap(); + } + let join = worker.event("step.dispatch").unwrap(); + let pins = join["pins"]["workspace"].as_array().unwrap(); + assert!( + pins.iter() + .any(|pin| pin["surface"] == "repo-b" && pin["revision_id"] == "rB") + ); + assert!( + pins.iter() + .any(|pin| pin["surface"] == "repo-a" && pin["revision_id"] == "rA") + ); + assert_eq!( + complete_agent(&mut worker, &join, &[("repo-b", "rJB"), ("repo-a", "rJA")]).unwrap()["status"], + "completed" + ); + } +} + +#[test] +fn overlapping_agent_conflict_survives_server_crash_and_resume() { + let fixture = LlmFixture::parallel_agents("overlap-crash", true); + let mut server = ServerGuard::start(&fixture); + let mut worker = attached_agent(&fixture, "before-crash"); + let run_id = start_run(&fixture); + assert_eq!(worker.event("step.dispatch").unwrap()["step_id"], "lane-b"); + server.kill(); + drop(worker); + let _restarted = ServerGuard::start(&fixture); + let mut replacement = attached_agent(&fixture, "after-crash"); + let resume = spawn_resume(&fixture, &run_id); + let retried = replacement.event("step.dispatch").unwrap(); + assert_eq!(retried["step_id"], "lane-b"); + assert_eq!(retried["attempt"], 2); + replacement.set_read_timeout(Some(Duration::from_millis(200))); + assert!(replacement.event("step.dispatch").is_err()); + replacement.set_read_timeout(None); + complete_agent(&mut replacement, &retried, &[("repo-b", "rB")]).unwrap(); + let lane_a = replacement.event("step.dispatch").unwrap(); + complete_agent(&mut replacement, &lane_a, &[("repo-b", "rA")]).unwrap(); + let join = replacement.event("step.dispatch").unwrap(); + complete_agent(&mut replacement, &join, &[("repo-b", "rJ")]).unwrap(); + assert!(resume.wait_with_output().unwrap().status.success()); +} + +#[test] +fn terminal_failure_drains_or_explains_every_live_sibling() { + for failed_step in ["lane-b", "lane-a"] { + let fixture = LlmFixture::parallel_terminal(&format!("terminal-{failed_step}")); + let _server = ServerGuard::start(&fixture); + let mut worker = attached_worker(&fixture, failed_step); + let run_id = start_run(&fixture); + let dispatches = [ + worker.event("step.dispatch").unwrap(), + worker.event("step.dispatch").unwrap(), + ]; + let failed = dispatches + .iter() + .find(|dispatch| dispatch["step_id"] == failed_step) + .unwrap(); + let sibling = dispatches + .iter() + .find(|dispatch| dispatch["step_id"] != failed_step) + .unwrap(); + assert_eq!( + complete_failure(&mut worker, failed).unwrap()["status"], + "parked" + ); + assert!( + !journal_entries(&fixture.data_dir) + .unwrap() + .iter() + .any(|entry| entry.entry_type == EntryType::RunCompleted) + ); + assert_eq!( + complete(&mut worker, sibling, json!({"answer": "done"})).unwrap()["status"], + "failed" + ); + assert_eq!( + journal_entries(&fixture.data_dir) + .unwrap() + .last() + .unwrap() + .entry_type, + EntryType::RunCompleted + ); + assert!( + worker + .request( + "step.complete", + json!({ + "run_id": run_id, + "step_id": sibling["step_id"], + "attempt": sibling["attempt"], + "idempotency_key": sibling["idempotency_key"], + "completionReason": "success", + "output": {"answer": "late"} + }) + ) + .is_err() + ); + assert_eq!( + journal_entries(&fixture.data_dir) + .unwrap() + .last() + .unwrap() + .entry_type, + EntryType::RunCompleted + ); + } + + let fixture = LlmFixture::parallel_terminal("terminal-disconnect"); + let _server = ServerGuard::start(&fixture); + let mut worker = attached_worker(&fixture, "disconnecting-worker"); + let run_id = start_run(&fixture); + let first = worker.event("step.dispatch").unwrap(); + worker.event("step.dispatch").unwrap(); + assert_eq!( + complete_failure(&mut worker, &first).unwrap()["status"], + "parked" + ); + drop(worker); + wait_until("sibling disconnect completion", || { + journal_entries(&fixture.data_dir).is_some_and(|entries| { + entries + .iter() + .filter(|entry| entry.entry_type == EntryType::StepCompleted) + .count() + == 2 + }) + }); + let mut control = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + assert_eq!( + control + .request("run.resume", json!({"run_id": run_id})) + .unwrap()["status"], + "failed" + ); + assert_eq!( + journal_entries(&fixture.data_dir) + .unwrap() + .last() + .unwrap() + .entry_type, + EntryType::RunCompleted + ); +} diff --git a/kernel/relayflowd/tests/parallel_driver.rs b/kernel/relayflowd/tests/parallel_driver.rs index 1e2790da6..b5299c1b0 100644 --- a/kernel/relayflowd/tests/parallel_driver.rs +++ b/kernel/relayflowd/tests/parallel_driver.rs @@ -57,6 +57,7 @@ impl JournalObserver for StartCrashObserver { struct RecordingDispatcher { crash_step: Option<&'static str>, mismatch_step: Option<&'static str>, + no_worker_step: Option<&'static str>, crash_fired: AtomicBool, agent_available: bool, calls: Mutex>, @@ -82,6 +83,21 @@ impl RecordingDispatcher { } } + fn detaching_at(step_id: &'static str) -> Self { + Self { + no_worker_step: Some(step_id), + ..Self::default() + } + } + + fn transient_mixed_failure() -> Self { + Self { + no_worker_step: Some("lane-b"), + mismatch_step: Some("lane-a"), + ..Self::default() + } + } + fn calls(&self) -> Vec { self.calls.lock().unwrap().clone() } @@ -98,7 +114,10 @@ impl StepDispatcher for RecordingDispatcher { fn dispatch(&self, dispatch: StepDispatch) -> anyhow::Result { self.calls.lock().unwrap().push(dispatch.clone()); - if self.mismatch_step == Some(dispatch.step_id.as_str()) { + if dispatch.attempt == 1 && self.no_worker_step == Some(dispatch.step_id.as_str()) { + return Ok(DispatchOutcome::NoWorker); + } + if dispatch.attempt == 1 && self.mismatch_step == Some(dispatch.step_id.as_str()) { return Ok(DispatchOutcome::PinMismatch { detail: "injected replacement pin mismatch".to_owned(), }); @@ -302,6 +321,53 @@ fn backpressured_or_mismatched_lane_does_not_drop_a_later_dispatch() { ["lane-b", "lane-a"], "a mismatched first lane must not discard the later dispatch" ); + + let directory = tempdir().unwrap(); + let dispatcher = Arc::new(RecordingDispatcher::detaching_at("lane-b")); + let observer = Arc::new(StartCrashObserver::default()); + let engine = Engine::with_runtime(directory.path(), dispatcher.clone(), observer.clone()); + engine.start(parallel_llm_spec(), "test", None).unwrap(); + assert_eq!( + dispatcher + .calls() + .iter() + .map(|dispatch| dispatch.step_id.as_str()) + .collect::>(), + ["lane-b", "lane-a"], + "a detach race on the first lane must not discard the later dispatch" + ); + let entries = engine + .journal_entries(&observer.run_id(), 1, usize::MAX) + .unwrap(); + assert!(entries.iter().any(|entry| { + entry.entry_type == EntryType::StepCompleted + && entry.step_id.as_deref() == Some("lane-b") + && serde_json::from_value::(entry.payload.clone()) + .unwrap() + .completion_reason + == CompletionReason::Crashed + })); + + let directory = tempdir().unwrap(); + let dispatcher = Arc::new(RecordingDispatcher::transient_mixed_failure()); + let observer = Arc::new(StartCrashObserver::default()); + let engine = Engine::with_runtime(directory.path(), dispatcher.clone(), observer); + assert_eq!( + engine + .start(parallel_llm_spec(), "test", None) + .unwrap() + .status, + RunStatus::Parked + ); + assert_eq!( + dispatcher + .calls() + .iter() + .map(|dispatch| (dispatch.step_id.as_str(), dispatch.attempt)) + .collect::>(), + [("lane-b", 1), ("lane-a", 1), ("lane-b", 2), ("lane-a", 2)], + "a compatible replacement must receive due retries without an external resume" + ); } #[test] From 2d928b450e9129044ca61f6c85534a69a1519db8 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 21:29:51 +0200 Subject: [PATCH 04/14] fix(kernel): close parallel dispatch admission gaps Session-Id: 01a062cc-f525-7d01-932e-a634815114c1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- .../relayflowd-core/src/machine/parallel.rs | 62 +++- .../src/machine/parallel_tests.rs | 18 ++ kernel/relayflowd-core/src/spec.rs | 76 ++++- kernel/relayflowd-core/src/spec/tests.rs | 55 ++++ kernel/relayflowd-journal/src/append.rs | 8 + kernel/relayflowd-journal/src/lib.rs | 45 ++- kernel/relayflowd/src/engine.rs | 75 ++++- kernel/relayflowd/src/engine/drive.rs | 89 ++++-- kernel/relayflowd/src/engine/effects.rs | 9 +- kernel/relayflowd/src/server.rs | 87 +----- kernel/relayflowd/src/server/protocol.rs | 95 ++++++ kernel/relayflowd/src/server/session.rs | 261 ++-------------- .../src/server/session/assignments.rs | 294 ++++++++++++++++++ .../relayflowd/src/server/session/matching.rs | 82 ++++- kernel/relayflowd/src/server/tests.rs | 9 +- kernel/relayflowd/src/server/wire.rs | 6 + kernel/relayflowd/src/worker.rs | 31 ++ kernel/relayflowd/tests/crash_resume.rs | 6 + .../tests/crash_resume/llm_support.rs | 47 ++- .../tests/crash_resume/parallel_lifecycle.rs | 1 + .../tests/crash_resume/protocol_admission.rs | 83 +++++ .../tests/crash_resume/surface_identity.rs | 111 +++++++ .../tests/crash_resume/worker_capacity.rs | 158 ++++++++++ sdk/src/journal-client.ts | 4 +- sdk/src/protocol.ts | 2 + sdk/src/validate.ts | 19 +- sdk/src/worker.ts | 8 +- sdk/tests/validate.test.ts | 31 ++ 28 files changed, 1377 insertions(+), 395 deletions(-) create mode 100644 kernel/relayflowd/src/server/protocol.rs create mode 100644 kernel/relayflowd/src/server/session/assignments.rs create mode 100644 kernel/relayflowd/tests/crash_resume/protocol_admission.rs create mode 100644 kernel/relayflowd/tests/crash_resume/surface_identity.rs create mode 100644 kernel/relayflowd/tests/crash_resume/worker_capacity.rs diff --git a/kernel/relayflowd-core/src/machine/parallel.rs b/kernel/relayflowd-core/src/machine/parallel.rs index 335b9c98c..032c7b115 100644 --- a/kernel/relayflowd-core/src/machine/parallel.rs +++ b/kernel/relayflowd-core/src/machine/parallel.rs @@ -6,13 +6,42 @@ //! attempt in backoff or waiting retains its reservation just like a live //! lease, preventing a crash/retry from turning into a last-write-wins fork. -use std::collections::BTreeSet; - use crate::{ - spec::{StepKind, StepSpec}, + spec::{StepKind, StepSpec, external_surface_identity}, state::{RunState, StepRuntime, StepState}, }; +#[derive(Clone, PartialEq, Eq)] +enum SurfaceIdentity { + Opaque(String), + External { + namespace: String, + components: Vec, + }, +} + +impl SurfaceIdentity { + fn conflicts(&self, other: &Self) -> bool { + match (self, other) { + (Self::Opaque(left), Self::Opaque(right)) => left == right, + ( + Self::External { + namespace: left_namespace, + components: left, + }, + Self::External { + namespace: right_namespace, + components: right, + }, + ) => { + left_namespace == right_namespace + && (left.starts_with(right) || right.starts_with(left)) + } + _ => false, + } + } +} + pub(super) fn runnable_batch(state: &RunState) -> Vec<(&StepSpec, &StepRuntime)> { let mut occupied = state .spec @@ -28,7 +57,7 @@ pub(super) fn runnable_batch(state: &RunState) -> Vec<(&StepSpec, &StepRuntime)> ) }) .flat_map(surface_keys) - .collect::>(); + .collect::>(); let mut selected = Vec::new(); for step in &state.spec.steps { let runtime = &state.steps[&step.id]; @@ -36,7 +65,10 @@ pub(super) fn runnable_batch(state: &RunState) -> Vec<(&StepSpec, &StepRuntime)> continue; } let surfaces = surface_keys(step).collect::>(); - if surfaces.iter().any(|surface| occupied.contains(surface)) { + if surfaces + .iter() + .any(|surface| occupied.iter().any(|held| surface.conflicts(held))) + { continue; } occupied.extend(surfaces); @@ -45,26 +77,28 @@ pub(super) fn runnable_batch(state: &RunState) -> Vec<(&StepSpec, &StepRuntime)> selected } -fn surface_keys(step: &StepSpec) -> impl Iterator + '_ { +fn surface_keys(step: &StepSpec) -> impl Iterator + '_ { let StepKind::Agent { surfaces, .. } = &step.kind else { return Vec::new().into_iter(); }; surfaces .workspace .iter() - .map(|surface| format!("workspace:{}", surface.surface)) + .map(|surface| SurfaceIdentity::Opaque(format!("workspace:{}", surface.surface))) .chain( surfaces .streams .iter() - .map(|surface| format!("stream:{}", surface.stream)), - ) - .chain( - surfaces - .external - .iter() - .map(|surface| format!("external:{surface}")), + .map(|surface| SurfaceIdentity::Opaque(format!("stream:{}", surface.stream))), ) + .chain(surfaces.external.iter().map(|surface| { + let (namespace, components) = external_surface_identity(surface) + .expect("validated specs have canonical external surfaces"); + SurfaceIdentity::External { + namespace, + components, + } + })) .collect::>() .into_iter() } diff --git a/kernel/relayflowd-core/src/machine/parallel_tests.rs b/kernel/relayflowd-core/src/machine/parallel_tests.rs index 769319754..96b1cb023 100644 --- a/kernel/relayflowd-core/src/machine/parallel_tests.rs +++ b/kernel/relayflowd-core/src/machine/parallel_tests.rs @@ -290,6 +290,24 @@ fn every_declared_mutable_surface_participates_in_conflict_selection() { } } +#[test] +fn external_ancestor_and_descendant_paths_conflict_but_siblings_do_not() { + let selected = |left: &str, right: &str| { + let spec = crate::RunSpec::parse(&json!({ + "steps": [ + {"id": "first", "type": "agent", "instruction": "a", "surfaces": {"external": [left]}}, + {"id": "second", "type": "agent", "instruction": "b", "surfaces": {"external": [right]}} + ] + })) + .unwrap(); + let state = RunState::fold("run", spec, &[]).unwrap(); + next_actions(&state, 10) + }; + assert_eq!(selected("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/provider/item", "/provider/item/child").len(), 2); + assert_eq!(selected("pr://github", "pr://github/example").len(), 2); + assert_eq!(selected("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/provider/a", "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/provider/b").len(), 4); +} + #[test] fn disjoint_agent_lanes_merge_pins_in_either_completion_order() { for order in [["lane-b", "lane-a"], ["lane-a", "lane-b"]] { diff --git a/kernel/relayflowd-core/src/spec.rs b/kernel/relayflowd-core/src/spec.rs index 97abb689d..f0b64e443 100644 --- a/kernel/relayflowd-core/src/spec.rs +++ b/kernel/relayflowd-core/src/spec.rs @@ -91,12 +91,12 @@ impl RunSpec { .as_ref() .is_some_and(|value| crate::event::validate_pattern(value).is_err()) || trigger.event_type.is_some() != trigger.dedupe_key_template.is_some() - // A stale_after_ms that does not fit in i64 would make - // the subscription un-stale-able — the liveness sweep - // stores that budget as i64. Refuse at parse time so - // the operator sees the error before submitting events - // rather than at the first arrival. i64::MAX ms is - // ~292M years; a value past that is not a real budget. + // A stale_after_ms that does not fit in i64 would make + // the subscription un-stale-able — the liveness sweep + // stores that budget as i64. Refuse at parse time so + // the operator sees the error before submitting events + // rather than at the first arrival. i64::MAX ms is + // ~292M years; a value past that is not a real budget. { return Err(SpecError::InvalidTrigger(trigger.id.clone())); } @@ -138,6 +138,16 @@ impl RunSpec { if cli.as_ref().is_some_and(|value| value.trim().is_empty()) { return Err(SpecError::EmptyStepCli(step.id.clone())); } + if let StepKind::Agent { surfaces, .. } = &step.kind { + for path in &surfaces.external { + if external_surface_identity(path).is_none() { + return Err(SpecError::InvalidExternalSurface { + step: step.id.clone(), + path: path.clone(), + }); + } + } + } step.retry.validate(&step.id)?; } @@ -169,6 +179,54 @@ impl RunSpec { } } +/// Filesystem-free canonical identity for a declared writeback target. +/// +/// The kernel cannot resolve host symlinks, so specs must already name a +/// lexical canonical path: no whitespace aliases, empty components, `.`, or +/// `..`. URI-like mount identities retain their scheme as a namespace. +pub(crate) fn external_surface_identity(path: &str) -> Option<(String, Vec)> { + if path.is_empty() || path.trim() != path { + return None; + } + let (namespace, tail) = if let Some(tail) = path.strip_prefix('/') { + ("/".to_owned(), tail) + } else if let Some(index) = path.find("://") { + let scheme = &path[..index]; + if scheme.is_empty() || scheme.contains('/') { + return None; + } + (path[..index + 3].to_owned(), &path[index + 3..]) + } else { + (String::new(), path) + }; + if tail.is_empty() { + return Some((namespace, Vec::new())); + } + let components = tail.split('/').map(str::to_owned).collect::>(); + (!components + .iter() + .any(|component| component.is_empty() || component == "." || component == "..")) + .then_some((namespace, components)) +} + +pub fn is_canonical_external_surface(path: &str) -> bool { + external_surface_identity(path).is_some() +} + +pub fn external_surface_contains(declared: &str, target: &str) -> bool { + let ( + Some((declared_namespace, declared_components)), + Some((target_namespace, target_components)), + ) = ( + external_surface_identity(declared), + external_surface_identity(target), + ) + else { + return false; + }; + declared_namespace == target_namespace && target_components.starts_with(&declared_components) +} + const STEP_COMMON_FIELDS: &[&str] = &[ "id", "type", @@ -483,7 +541,9 @@ pub enum SpecError { EmptyCli, #[error("trigger {0} must declare a non-empty id and executor")] InvalidTrigger(String), - #[error("trigger {id} declared stale_after_ms={ms} which does not fit in i64 (~292M years); the liveness sweep cannot represent that budget")] + #[error( + "trigger {id} declared stale_after_ms={ms} which does not fit in i64 (~292M years); the liveness sweep cannot represent that budget" + )] TriggerStaleAfterMsOutOfRange { id: String, ms: u64 }, #[error("duplicate trigger id: {0}")] DuplicateTrigger(String), @@ -491,6 +551,8 @@ pub enum SpecError { EmptyStepId, #[error("step {0} cli cannot be empty")] EmptyStepCli(String), + #[error("agent step {step} declares non-canonical external surface {path:?}")] + InvalidExternalSurface { step: String, path: String }, #[error("duplicate step id: {0}")] DuplicateStep(String), #[error("step {0} must allow at least one iteration")] diff --git a/kernel/relayflowd-core/src/spec/tests.rs b/kernel/relayflowd-core/src/spec/tests.rs index 6412b9c51..81d77f416 100644 --- a/kernel/relayflowd-core/src/spec/tests.rs +++ b/kernel/relayflowd-core/src/spec/tests.rs @@ -159,6 +159,61 @@ fn the_full_ladder_parses_in_the_one_dialect() { assert_eq!(spec.triggers[0].executor, "worker-a"); } +#[test] +fn external_surface_paths_must_have_one_canonical_spelling() { + for path in [ + "/provider/./item", + "/provider/../item", + "/provider//item", + "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/provider/item/", + " pr://github/example", + ] { + let spec = RunSpec::parse(&json!({ + "steps": [{ + "id": "agent", + "type": "agent", + "instruction": "write", + "surfaces": {"external": [path]} + }] + })) + .unwrap(); + assert!( + matches!( + spec.validate(), + Err(SpecError::InvalidExternalSurface { path: invalid, .. }) if invalid == path + ), + "accepted non-canonical surface {path:?}" + ); + } + for path in ["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/provider/item", "pr://github/example", "provider/item"] { + let spec = RunSpec::parse(&json!({ + "steps": [{ + "id": "agent", + "type": "agent", + "instruction": "write", + "surfaces": {"external": [path]} + }] + })) + .unwrap(); + assert!( + spec.validate().is_ok(), + "rejected canonical surface {path:?}" + ); + } + assert!(external_surface_contains( + "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/provider/item", + "/provider/item/child" + )); + assert!(!external_surface_contains( + "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/provider/item", + "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/provider/other" + )); + assert!(!external_surface_contains( + "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/provider/item", + "/provider/./item" + )); +} + #[test] fn preflight_data_is_fail_closed() { let malformed = RunSpec::parse(&json!({ diff --git a/kernel/relayflowd-journal/src/append.rs b/kernel/relayflowd-journal/src/append.rs index 9a1e6c747..0de806589 100644 --- a/kernel/relayflowd-journal/src/append.rs +++ b/kernel/relayflowd-journal/src/append.rs @@ -30,6 +30,14 @@ impl SqliteJournal { let transaction = self .connection .transaction_with_behavior(TransactionBehavior::Immediate)?; + let terminal: bool = transaction.query_row( + "SELECT EXISTS(SELECT 1 FROM entries WHERE entry_type = ?1)", + [EntryType::RunCompleted.as_str()], + |row| row.get(0), + )?; + if terminal { + return Err(JournalStoreError::RunTerminal(self.run_id.clone())); + } let persisted = insert_entry(&transaction, &self.run_id, current_segment, entry)?; transaction.commit()?; Ok(persisted) diff --git a/kernel/relayflowd-journal/src/lib.rs b/kernel/relayflowd-journal/src/lib.rs index debd1dc65..fd0fe81c3 100644 --- a/kernel/relayflowd-journal/src/lib.rs +++ b/kernel/relayflowd-journal/src/lib.rs @@ -171,6 +171,16 @@ impl SqliteJournal { .map_err(Into::into) } + pub fn is_terminal(&self) -> Result { + self.connection + .query_row( + "SELECT EXISTS(SELECT 1 FROM entries WHERE entry_type = ?1)", + [relayflowd_core::EntryType::RunCompleted.as_str()], + |row| row.get(0), + ) + .map_err(Into::into) + } + fn scan_where( &self, sql: &str, @@ -248,6 +258,8 @@ pub enum JournalStoreError { WrongRun { expected: String, actual: String }, #[error("journal entry targets segment {actual}, current segment is {expected}")] WrongSegment { expected: i64, actual: i64 }, + #[error("run {0} is terminal and cannot accept journal entries")] + RunTerminal(String), #[error("stream {stream} expected offset {expected}, received {actual}")] InvalidStreamOffset { stream: String, @@ -274,7 +286,7 @@ mod tests { use relayflowd_core::{ Budget, EffectConfirmedPayload, EffectRecordedPayload, EntryType, EpochSummaryPayload, - RunSpawnedPayload, + RunCompletedPayload, RunCompletionReason, RunSpawnedPayload, }; use serde_json::json; use tempfile::tempdir; @@ -336,6 +348,37 @@ mod tests { assert!(error.0.contains("readonly") || error.0.contains("read-only")); } + #[test] + fn terminal_run_refuses_every_later_entry_atomically() { + let (_directory, mut journal) = created(); + journal + .append(&JournalEntry::new( + EntryType::RunCompleted, + "run", + None, + None, + 10, + RunCompletedPayload { + completion_reason: RunCompletionReason::Success, + failed_step_id: None, + budget_total: Budget::default(), + }, + )) + .unwrap(); + let error = journal + .append(&JournalEntry::new( + EntryType::StreamAppended, + "run", + None, + None, + 11, + json!({}), + )) + .unwrap_err(); + assert!(error.0.contains("terminal"), "{error:?}"); + assert_eq!(journal.scan_all().unwrap().len(), 1); + } + fn election(attempt: u32) -> JournalEntry { JournalEntry::new( EntryType::EffectRecorded, diff --git a/kernel/relayflowd/src/engine.rs b/kernel/relayflowd/src/engine.rs index a9ee9ae28..4aff4d16d 100644 --- a/kernel/relayflowd/src/engine.rs +++ b/kernel/relayflowd/src/engine.rs @@ -12,6 +12,23 @@ use relayflowd_journal::{Registry, SqliteJournal}; use sha2::{Digest, Sha256}; use ulid::Ulid; +#[derive(Debug)] +pub struct RunTerminalError { + pub run_id: String, +} + +impl std::fmt::Display for RunTerminalError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + formatter, + "run {} is terminal and cannot accept mutations", + self.run_id + ) + } +} + +impl std::error::Error for RunTerminalError {} + use crate::clock::WallClock; use crate::worker::{JournalObserver, StepDispatcher}; @@ -249,6 +266,7 @@ impl Engine { journal: &mut SqliteJournal, entry: &JournalEntry, ) -> Result { + self.ensure_journal_mutable(journal)?; let persisted = journal.append(entry).map_err(|error| anyhow!(error))?; if let Some(observer) = &self.observer { observer.appended(&persisted); @@ -256,7 +274,25 @@ impl Engine { Ok(persisted) } - fn assign_executor(&self, entry: &mut JournalEntry) -> Result<()> { + pub fn ensure_run_mutable(&self, run_id: &str) -> Result<()> { + let journal = self.open_run(run_id)?; + self.ensure_journal_mutable(&journal) + } + + fn ensure_journal_mutable(&self, journal: &SqliteJournal) -> Result<()> { + if journal + .is_terminal() + .context("read terminal admission state")? + { + return Err(RunTerminalError { + run_id: journal.run_id().to_owned(), + } + .into()); + } + Ok(()) + } + + fn assign_executor(&self, state: &RunState, entry: &mut JournalEntry) -> Result<()> { if entry.entry_type != EntryType::StepAttemptStarted { return Ok(()); } @@ -265,10 +301,19 @@ impl Engine { if payload.step_type == relayflowd_core::StepType::Deterministic { return Ok(()); } + let step_id = entry + .step_id + .as_deref() + .context("out-of-band start has no step id")?; + let step = state + .spec + .step(step_id) + .with_context(|| format!("run has no step {step_id}"))?; + let attempt = entry.attempt.context("out-of-band start has no attempt")?; if let Some(executor) = self .dispatcher .as_ref() - .and_then(|dispatcher| dispatcher.executor(payload.step_type)) + .and_then(|dispatcher| dispatcher.reserved_executor(&state.run_id, step, attempt)) { payload.executor = executor; entry.payload = serde_json::to_value(payload)?; @@ -293,7 +338,8 @@ impl Engine { .spec .step(step_id) .with_context(|| format!("run has no step {step_id}"))?; - payload.pins = self.resolve_agent_pins(step, &payload.pins)?; + let attempt = entry.attempt.context("agent start has no attempt")?; + payload.pins = self.resolve_agent_pins(&state.run_id, step, attempt, &payload.pins)?; validate_agent_pins(step, &payload.pins)?; entry.payload = serde_json::to_value(payload)?; Ok(()) @@ -306,7 +352,9 @@ impl Engine { /// agent steps may therefore declare entirely different surfaces. fn resolve_agent_pins( &self, + run_id: &str, step: &relayflowd_core::StepSpec, + attempt: u32, carried: &relayflowd_core::Pins, ) -> Result { let StepKind::Agent { surfaces, .. } = &step.kind else { @@ -331,7 +379,7 @@ impl Engine { self.dispatcher .as_ref() .context("agent dispatch requires an attached worker")? - .starting_pins(step)? + .reserved_starting_pins(run_id, step, attempt)? }; // Project onto the declared surfaces, in declared order: a surface this // step does not declare is outside its contract (Appendix A rule 1) and @@ -364,23 +412,18 @@ impl Engine { }) } - /// Preflight for Appendix A rule 2: can the attached worker pin every - /// surface this agent step declares that the chain has not already pinned? - /// A worker that cannot is not a compatible worker for this step — the run - /// parks until one that can attaches, rather than failing mid-drive with an - /// untyped error after `run.start` has already journaled the spawn. - pub(super) fn agent_pins_available( - &self, + pub(super) fn dispatch_required_pins( state: &RunState, step: &relayflowd_core::StepSpec, - ) -> bool { + ) -> relayflowd_core::Pins { + if !matches!(step.kind, StepKind::Agent { .. }) { + return relayflowd_core::Pins::default(); + } let carried = relayflowd_core::carried_pins_for(state.current_pins.as_ref(), step); - let carried = state.steps[&step.id] + state.steps[&step.id] .last_start_pins .clone() - .unwrap_or(carried); - self.resolve_agent_pins(step, &carried) - .is_ok_and(|pins| validate_agent_pins(step, &pins).is_ok()) + .unwrap_or(carried) } fn open_run(&self, run_id: &str) -> Result { diff --git a/kernel/relayflowd/src/engine/drive.rs b/kernel/relayflowd/src/engine/drive.rs index f77802c74..9bee4c6f4 100644 --- a/kernel/relayflowd/src/engine/drive.rs +++ b/kernel/relayflowd/src/engine/drive.rs @@ -67,24 +67,50 @@ impl Engine { pause_consumed = true; thread::sleep(Duration::from_secs(300)); } - if step.step_type() != relayflowd_core::StepType::Deterministic - && !self.step_is_dispatchable(&state, step) - { + let attempt = entry.attempt.expect("a step start has an attempt"); + let admitted = + if step.step_type() == relayflowd_core::StepType::Deterministic { + true + } else if let Some(dispatcher) = &self.dispatcher { + dispatcher.reserve_dispatch( + &state.run_id, + step, + attempt, + &Self::dispatch_required_pins(&state, step), + )? + } else { + false + }; + if !admitted { // Admission is per pair. A lane with no // compatible worker remains Runnable, while // later independent lanes still get their // journal-first handoff. - skipped_dispatches.insert(( - step_id.to_owned(), - entry.attempt.expect("a step start has an attempt"), - )); + skipped_dispatches.insert((step_id.to_owned(), attempt)); backpressured = true; continue; } } - self.prepare_start_entry(&state, &mut entry)?; - self.assign_executor(&mut entry)?; - self.append(&mut journal, &entry)?; + let prepared = (|| -> Result<()> { + self.prepare_start_entry(&state, &mut entry)?; + self.assign_executor(&state, &mut entry)?; + self.append(&mut journal, &entry)?; + Ok(()) + })(); + if let Err(error) = prepared { + if let (Some(dispatcher), Some(step_id), Some(attempt)) = ( + self.dispatcher.as_ref(), + entry.step_id.as_deref(), + entry.attempt, + ) { + dispatcher.release_dispatch_reservation( + &state.run_id, + step_id, + attempt, + ); + } + return Err(error); + } } Action::ExecDeterministic { step, attempt } => { let result = exec_det::execute(&step); @@ -149,8 +175,21 @@ impl Engine { lease_deadline_ms, }) }) - .transpose()? - .unwrap_or(DispatchOutcome::NoWorker); + .transpose(); + let outcome = match outcome { + Ok(Some(outcome)) => outcome, + Ok(None) => DispatchOutcome::NoWorker, + Err(error) => { + if let Some(dispatcher) = &self.dispatcher { + dispatcher.release_dispatch_reservation( + &state.run_id, + &step.id, + attempt, + ); + } + return Err(error); + } + }; match outcome { DispatchOutcome::Dispatched => { // Record operational backpressure after each @@ -166,6 +205,13 @@ impl Engine { dispatched = true; } DispatchOutcome::NoWorker => { + if let Some(dispatcher) = &self.dispatcher { + dispatcher.release_dispatch_reservation( + &state.run_id, + &step.id, + attempt, + ); + } // Preflight admitted the whole batch, so this // is a detach race. Explain the unhanded lease // as crashed, leave it retryable, and continue: @@ -184,6 +230,13 @@ impl Engine { handoff_failed = true; } DispatchOutcome::PinMismatch { detail } => { + if let Some(dispatcher) = &self.dispatcher { + dispatcher.release_dispatch_reservation( + &state.run_id, + &step.id, + attempt, + ); + } // Appendix A rule 2: a worker standing at // revisions other than the journaled pins // fails closed. Continue the batch so an @@ -304,18 +357,6 @@ impl Engine { Ok(()) } - fn step_is_dispatchable(&self, state: &RunState, step: &relayflowd_core::StepSpec) -> bool { - let step_type = step.step_type(); - let available = self - .dispatcher - .as_ref() - .is_some_and(|dispatcher| dispatcher.available(step_type)); - if !available || step_type != relayflowd_core::StepType::Agent { - return available; - } - self.agent_pins_available(state, step) - } - fn park_idle_run(&self, state: &RunState) -> Result<()> { let active_lease = state .spec diff --git a/kernel/relayflowd/src/engine/effects.rs b/kernel/relayflowd/src/engine/effects.rs index a40336583..bf393d5b0 100644 --- a/kernel/relayflowd/src/engine/effects.rs +++ b/kernel/relayflowd/src/engine/effects.rs @@ -124,7 +124,14 @@ impl Engine { let StepKind::Agent { surfaces, .. } = &step.kind else { bail!("step {step_id} is not an agent step") }; - if !surfaces.external.iter().any(|path| path == surface_path) { + if !relayflowd_core::is_canonical_external_surface(surface_path) { + bail!("effect path {surface_path} is not canonical") + } + if !surfaces + .external + .iter() + .any(|declared| relayflowd_core::external_surface_contains(declared, surface_path)) + { bail!("effect path {surface_path} is not declared by agent step {step_id}") } let StepState::Running { diff --git a/kernel/relayflowd/src/server.rs b/kernel/relayflowd/src/server.rs index eb5cd8ee2..c827bb9da 100644 --- a/kernel/relayflowd/src/server.rs +++ b/kernel/relayflowd/src/server.rs @@ -2,7 +2,6 @@ use std::path::Path; use anyhow::{Context, Result}; use relayflowd_core::{CompletionReason, PROTOCOL_VERSION, RunSpec, StepType}; -use serde::{Serialize, de::DeserializeOwned}; use serde_json::{Value, json}; use crate::{Engine, OutOfBandCompletion}; @@ -26,12 +25,12 @@ pub use client::{cancel_via_socket, resume_via_socket}; mod wire; use wire::*; +mod protocol; +use protocol::*; #[cfg(all(test, unix))] mod tests; -type ProtocolResult = std::result::Result; - #[cfg(unix)] pub fn serve(data_dir: &Path) -> Result<()> { use std::{ @@ -117,29 +116,6 @@ pub fn serve(_data_dir: &Path) -> Result<()> { } #[cfg(unix)] -fn handle_line( - data_dir: &Path, - hub: &std::sync::Arc, - connection_id: u64, - writer: &SharedWriter, - line: &str, -) -> Response { - let request: Request = match serde_json::from_str(line) { - Ok(request) => request, - Err(error) => return error_response(Value::Null, "bad_request", error.to_string()), - }; - let id = request.id.clone(); - match handle_request(data_dir, hub, connection_id, writer, request) { - Ok(result) => Response { - id, - ok: true, - result: Some(result), - error: None, - }, - Err((code, message)) => error_response(id, code, message), - } -} - #[cfg(unix)] fn handle_request( data_dir: &Path, @@ -166,6 +142,8 @@ fn handle_request( let params: RunStartParams = decode_params(request.params)?; let spec = RunSpec::parse(¶ms.spec) .map_err(|error| ("invalid_spec", error.to_string()))?; + spec.validate() + .map_err(|error| ("invalid_spec", error.to_string()))?; to_value( engine .start(spec, "protocol-v0", None) @@ -218,6 +196,9 @@ fn handle_request( if params.step_types.is_empty() { return Err(("bad_request", "worker must accept a step type".to_owned())); } + if params.capacity == 0 { + return Err(("bad_request", "worker capacity must be positive".to_owned())); + } // Appendix A rule 2: an agent attempt is journaled with the opaque // revisions the worker reports. A worker that accepts agent steps // and reports no surface at all can never supply them, and that is @@ -236,6 +217,7 @@ fn handle_request( connection_id, params.worker_id.clone(), params.step_types, + params.capacity, params.pins, writer.clone(), ); @@ -243,6 +225,9 @@ fn handle_request( } "step.heartbeat" => { let params: StepHeartbeatParams = decode_params(request.params)?; + let lock = hub.run_lock(¶ms.run_id); + let _guard = lock.lock().expect("run lock"); + ensure_mutable(&engine, ¶ms.run_id)?; let (deadline, run_deadline) = hub .heartbeat( connection_id, @@ -281,6 +266,7 @@ fn handle_request( ); let lock = hub.run_lock(¶ms.run_id); let _guard = lock.lock().expect("run lock"); + ensure_mutable(&engine, ¶ms.run_id)?; let worker_id = hub .completion_worker(connection_id, &key) .map_err(protocol_conflict)?; @@ -326,6 +312,7 @@ fn handle_request( ); let lock = hub.run_lock(¶ms.run_id); let _guard = lock.lock().expect("run lock"); + ensure_mutable(&engine, ¶ms.run_id)?; let worker_id = hub .completion_worker(connection_id, &key) .map_err(protocol_conflict)?; @@ -352,6 +339,7 @@ fn handle_request( ); let lock = hub.run_lock(¶ms.run_id); let _guard = lock.lock().expect("run lock"); + ensure_mutable(&engine, ¶ms.run_id)?; let worker_id = hub .completion_worker(connection_id, &key) .map_err(protocol_conflict)?; @@ -371,6 +359,7 @@ fn handle_request( let params: EventEmitParams = decode_params(request.params)?; let lock = hub.run_lock(¶ms.run_id); let _guard = lock.lock().expect("run lock"); + ensure_mutable(&engine, ¶ms.run_id)?; let matched = engine .emit_event(¶ms.run_id, ¶ms.event_key, params.payload) .map_err(internal_error)?; @@ -390,6 +379,7 @@ fn handle_request( let params: StreamAppendParams = decode_params(request.params)?; let lock = hub.run_lock(¶ms.run_id); let _guard = lock.lock().expect("run lock"); + ensure_mutable(&engine, ¶ms.run_id)?; let offset = engine .append_stream( ¶ms.run_id, @@ -487,48 +477,3 @@ fn watch_with_replay( } Ok(json!({"watching": run_id})) } - -fn decode_params(params: Value) -> ProtocolResult { - serde_json::from_value(params).map_err(|error| ("bad_request", error.to_string())) -} - -fn to_value(value: impl Serialize) -> ProtocolResult { - serde_json::to_value(value).map_err(|error| internal_error(error.into())) -} - -fn protocol_conflict(error: anyhow::Error) -> (&'static str, String) { - ("lease_conflict", error.to_string()) -} - -fn internal_error(error: anyhow::Error) -> (&'static str, String) { - let journal_failure = error.chain().any(|cause| { - cause.is::() - || cause.is::() - }); - let code = if journal_failure { - "journal_write_failed" - } else { - "internal" - }; - (code, format!("{error:#}")) -} - -fn error_response(id: Value, code: &str, message: String) -> Response { - Response { - id, - ok: false, - result: None, - error: Some(ProtocolError { - code: code.to_owned(), - message, - }), - } -} - -fn now_ms() -> i64 { - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_millis() - .min(i64::MAX as u128) as i64 -} diff --git a/kernel/relayflowd/src/server/protocol.rs b/kernel/relayflowd/src/server/protocol.rs new file mode 100644 index 000000000..883b41625 --- /dev/null +++ b/kernel/relayflowd/src/server/protocol.rs @@ -0,0 +1,95 @@ +//! Wire response construction and typed protocol error mapping. + +use std::{path::Path, sync::Arc}; + +use serde::{Serialize, de::DeserializeOwned}; +use serde_json::Value; + +use super::{ + Engine, ProtocolHub, Request, Response, SharedWriter, handle_request, wire::ProtocolError, +}; + +pub(super) type ProtocolResult = std::result::Result; + +pub(super) fn handle_line( + data_dir: &Path, + hub: &Arc, + connection_id: u64, + writer: &SharedWriter, + line: &str, +) -> Response { + let request: Request = match serde_json::from_str(line) { + Ok(request) => request, + Err(error) => return error_response(Value::Null, "bad_request", error.to_string()), + }; + let id = request.id.clone(); + match handle_request(data_dir, hub, connection_id, writer, request) { + Ok(result) => Response { + id, + ok: true, + result: Some(result), + error: None, + }, + Err((code, message)) => error_response(id, code, message), + } +} + +pub(super) fn decode_params(params: Value) -> ProtocolResult { + serde_json::from_value(params).map_err(|error| ("bad_request", error.to_string())) +} + +pub(super) fn to_value(value: impl Serialize) -> ProtocolResult { + serde_json::to_value(value).map_err(|error| internal_error(error.into())) +} + +pub(super) fn protocol_conflict(error: anyhow::Error) -> (&'static str, String) { + ("lease_conflict", error.to_string()) +} + +pub(super) fn ensure_mutable(engine: &Engine, run_id: &str) -> ProtocolResult<()> { + engine.ensure_run_mutable(run_id).map_err(mutation_error) +} + +fn mutation_error(error: anyhow::Error) -> (&'static str, String) { + if error + .downcast_ref::() + .is_some() + { + ("run_terminal", error.to_string()) + } else { + internal_error(error) + } +} + +pub(super) fn internal_error(error: anyhow::Error) -> (&'static str, String) { + let journal_failure = error.chain().any(|cause| { + cause.is::() + || cause.is::() + }); + let code = if journal_failure { + "journal_write_failed" + } else { + "internal" + }; + (code, format!("{error:#}")) +} + +pub(super) fn error_response(id: Value, code: &str, message: String) -> Response { + Response { + id, + ok: false, + result: None, + error: Some(ProtocolError { + code: code.to_owned(), + message, + }), + } +} + +pub(super) fn now_ms() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_millis() + .min(i64::MAX as u128) as i64 +} diff --git a/kernel/relayflowd/src/server/session.rs b/kernel/relayflowd/src/server/session.rs index bac789af8..9ac43d93b 100644 --- a/kernel/relayflowd/src/server/session.rs +++ b/kernel/relayflowd/src/server/session.rs @@ -5,25 +5,26 @@ use std::{ sync::{Arc, Mutex}, }; -use anyhow::{Context, Result, bail}; -use relayflowd_core::{CompletionReason, JournalEntry, Pins, StepKind, StepSpec, StepType}; +use anyhow::Result; +use relayflowd_core::{CompletionReason, JournalEntry, Pins, StepType}; use serde_json::json; -use crate::worker::{DispatchOutcome, JournalObserver, LeaseProbe, StepDispatch, StepDispatcher}; +use crate::worker::JournalObserver; const LEASE_RENEWAL_MS: i64 = 30_000; type Writer = Arc>; type AssignmentKey = (String, String, u32); +mod assignments; mod matching; -use matching::{pin_value_mismatch, select_worker, worker_holds}; #[derive(Clone)] struct Worker { connection_id: u64, worker_id: String, step_types: Vec, + capacity: usize, pins: Pins, writer: Writer, } @@ -55,6 +56,11 @@ struct Assignment { lease_deadline_ms: i64, } +#[derive(Clone)] +struct Reservation { + connection_id: u64, +} + #[derive(Debug, Clone)] pub struct AbandonedLease { pub run_id: String, @@ -76,6 +82,7 @@ struct Sessions { workers: Vec, watchers: BTreeMap>, assignments: BTreeMap, + reservations: BTreeMap, } #[derive(Default)] @@ -104,6 +111,7 @@ impl ProtocolHub { connection_id: u64, worker_id: String, step_types: Vec, + capacity: usize, pins: Pins, writer: Writer, ) { @@ -115,6 +123,7 @@ impl ProtocolHub { connection_id, worker_id, step_types, + capacity, pins, writer, }); @@ -208,101 +217,6 @@ impl ProtocolHub { } } - pub fn heartbeat( - &self, - connection_id: u64, - key: &AssignmentKey, - lease_id: &str, - now_ms: i64, - ) -> Result<(i64, i64)> { - let mut sessions = self.sessions.lock().expect("protocol sessions lock"); - let assignment_deadline = { - let assignment = sessions - .assignments - .get_mut(key) - .context("attempt has no active worker lease")?; - if assignment.connection_id != connection_id || assignment.lease_id != lease_id { - bail!("heartbeat does not match the active worker lease") - } - assignment.lease_deadline_ms = now_ms.saturating_add(LEASE_RENEWAL_MS); - assignment.lease_deadline_ms - }; - let run_deadline = sessions - .assignments - .iter() - .filter(|((run_id, _, _), _)| run_id == &key.0) - .map(|(_, assignment)| assignment.lease_deadline_ms) - .min() - .expect("the renewed assignment is still present"); - Ok((assignment_deadline, run_deadline)) - } - - pub fn completion_worker(&self, connection_id: u64, key: &AssignmentKey) -> Result { - let sessions = self.sessions.lock().expect("protocol sessions lock"); - let assignment = sessions - .assignments - .get(key) - .context("attempt has no active worker lease")?; - if assignment.connection_id != connection_id { - bail!("completion came from a worker that does not hold the lease") - } - Ok(assignment.worker_id.clone()) - } - - pub fn finish(&self, key: &AssignmentKey) { - self.sessions - .lock() - .expect("protocol sessions lock") - .assignments - .remove(key); - } - - /// Release every in-memory lease after the journal has durably made the - /// run terminal. Calling this repeatedly is intentionally harmless. Under - /// parallel dispatch a terminal run can hold several live leases at once, - /// so this releases the whole run rather than one attempt. - pub fn finish_run(&self, run_id: &str) { - self.sessions - .lock() - .expect("protocol sessions lock") - .assignments - .retain(|(assigned_run, _, _), _| assigned_run != run_id); - } - - /// The run registry has one operational wake deadline even when the - /// journal has several live leases. It must track the earliest assignment - /// so a heartbeat on one lane cannot hide an earlier sibling expiry. - pub fn earliest_lease_deadline(&self, run_id: &str) -> Option { - self.sessions - .lock() - .expect("protocol sessions lock") - .assignments - .iter() - .filter(|((assigned_run, _, _), _)| assigned_run == run_id) - .map(|(_, assignment)| assignment.lease_deadline_ms) - .min() - } - - /// Assignments whose (heartbeat-renewed) lease deadline has passed. The - /// worker may still hold an open socket — a hung worker is exactly the - /// case the expiry reconciler exists for. Assignments are NOT removed - /// here; they are released via `finish` only once the abandonment is - /// durably journaled, so a failed journal write is retried next sweep. - pub fn expired_assignments(&self, now_ms: i64) -> Vec { - self.sessions - .lock() - .expect("protocol sessions lock") - .assignments - .iter() - .filter(|(_, assignment)| now_ms >= assignment.lease_deadline_ms) - .map(|((run_id, step_id, attempt), _)| AbandonedLease { - run_id: run_id.clone(), - step_id: step_id.clone(), - attempt: *attempt, - }) - .collect() - } - /// Retain an abandonment whose journal append failed, for reconciler retry. pub fn queue_abandonment(&self, lease: AbandonedLease, reason: CompletionReason) { self.pending_abandonments @@ -336,9 +250,9 @@ impl ProtocolHub { (assignment.connection_id == connection_id).then_some(key.clone()) }) .collect::>(); - for key in &keys { - sessions.assignments.remove(key); - } + sessions + .reservations + .retain(|_, reservation| reservation.connection_id != connection_id); keys.into_iter() .map(|(run_id, step_id, attempt)| AbandonedLease { run_id, @@ -349,142 +263,19 @@ impl ProtocolHub { } } -impl StepDispatcher for ProtocolHub { - fn executor(&self, step_type: StepType) -> Option { - self.sessions - .lock() - .expect("protocol sessions lock") - .workers - .iter() - .find(|worker| worker.step_types.contains(&step_type)) - .map(|worker| worker.worker_id.clone()) - } - - fn available(&self, step_type: StepType) -> bool { - self.executor(step_type).is_some() - } - - fn starting_pins(&self, step: &StepSpec) -> Result { - let sessions = self.sessions.lock().expect("protocol sessions lock"); - // Same selection rule as `dispatch` — the first worker handling the - // class — so the pins journaled at start belong to the worker that - // receives the attempt. `dispatch` re-checks the worker id it resolved - // against the pin source and declines rather than dispatching to a - // worker whose starting state was never journaled. - let worker = - select_worker(&sessions, StepType::Agent).context("no agent worker is attached")?; - let StepKind::Agent { surfaces, .. } = &step.kind else { - return Ok(Pins::default()); - }; - let workspace = surfaces - .workspace - .iter() - .map(|surface| { - worker - .pins - .workspace - .iter() - .find(|pin| pin.surface == surface.surface) - .cloned() - .with_context(|| { - format!( - "worker {} omitted revision for surface {}", - worker.worker_id, surface.surface - ) - }) - }) - .collect::>>()?; - let streams = surfaces - .streams - .iter() - .map(|surface| { - worker - .pins - .streams - .iter() - .find(|pin| pin.stream == surface.stream) - .cloned() - .with_context(|| { - format!( - "worker {} omitted read offset for stream {}", - worker.worker_id, surface.stream - ) - }) - }) - .collect::>>()?; - Ok(Pins { workspace, streams }) - } - - fn dispatch(&self, dispatch: StepDispatch) -> Result { - let mut sessions = self.sessions.lock().expect("protocol sessions lock"); - let Some(worker) = select_worker(&sessions, dispatch.step_type).cloned() else { - return Ok(DispatchOutcome::NoWorker); - }; - // Appendix A rule 2: the pins journaled at start are the state this - // attempt must begin from, and they were sourced from whichever worker - // `select_worker` returned then. If a detach or a second attachment has - // changed that answer, the worker now selected may never have reported - // those surfaces — dispatching would hand it a starting state it cannot - // honor. Decline instead; the run parks and re-dispatches from pins the - // holding worker actually reported. - if !worker_holds(&worker, &dispatch.pins) { - return Ok(DispatchOutcome::NoWorker); - } - // Holding the surface *names* is not holding the state. Unless this - // dispatch is itself the instruction to move (a `reset` retry carries - // `restore_pins`), the worker must already be at the exact revisions - // and offsets the attempt was elected against; a replacement standing - // at different ones would start from unjournaled state. - if let Some(detail) = pin_value_mismatch(&worker, &dispatch) { - return Ok(DispatchOutcome::PinMismatch { detail }); - } - write_frame( - &worker.writer, - &json!({"event": "step.dispatch", "data": dispatch}), - ) - .with_context(|| format!("dispatch step to worker {}", worker.worker_id))?; - let key = ( - dispatch.run_id.clone(), - dispatch.step_id.clone(), - dispatch.attempt, - ); - sessions.assignments.insert( - key, - Assignment { - connection_id: worker.connection_id, - worker_id: worker.worker_id, - lease_id: dispatch.lease_id, - lease_deadline_ms: dispatch.lease_deadline_ms, - }, - ); - Ok(DispatchOutcome::Dispatched) - } - - fn active_lease_deadline(&self, run_id: &str, step_id: &str, attempt: u32) -> Option { - self.sessions - .lock() - .expect("protocol sessions lock") - .assignments - .get(&(run_id.to_owned(), step_id.to_owned(), attempt)) - .map(|assignment| assignment.lease_deadline_ms) - } -} - -impl LeaseProbe for ProtocolHub { - fn lease_active(&self, run_id: &str, step_id: &str, attempt: u32, now_ms: i64) -> bool { - let key = (run_id.to_owned(), step_id.to_owned(), attempt); - self.sessions - .lock() - .expect("protocol sessions lock") - .assignments - .get(&key) - .is_some_and(|assignment| now_ms < assignment.lease_deadline_ms) - } -} - impl JournalObserver for ProtocolHub { fn appended(&self, entry: &JournalEntry) { let mut sessions = self.sessions.lock().expect("protocol sessions lock"); + // Capacity returns only after the completion is a durable journal + // fact. This callback runs after append and before the driver elects + // later runnable work, so a freed slot can be reused immediately. + if entry.entry_type == relayflowd_core::EntryType::StepCompleted + && let (Some(step_id), Some(attempt)) = (&entry.step_id, entry.attempt) + { + sessions + .assignments + .remove(&(entry.run_id.clone(), step_id.clone(), attempt)); + } let Some(watchers) = sessions.watchers.get_mut(&entry.run_id) else { return; }; diff --git a/kernel/relayflowd/src/server/session/assignments.rs b/kernel/relayflowd/src/server/session/assignments.rs new file mode 100644 index 000000000..41f94d5bb --- /dev/null +++ b/kernel/relayflowd/src/server/session/assignments.rs @@ -0,0 +1,294 @@ +//! Capacity reservations, deterministic placement, leases, and dispatch. + +use anyhow::{Context, Result, bail}; +use relayflowd_core::{Pins, StepKind, StepSpec, StepType}; +use serde_json::json; + +use super::{ + AbandonedLease, Assignment, AssignmentKey, LEASE_RENEWAL_MS, ProtocolHub, Reservation, + matching::{pin_value_mismatch, select_worker, select_worker_for_step, worker_holds}, + write_frame, +}; +use crate::worker::{DispatchOutcome, LeaseProbe, StepDispatch, StepDispatcher}; + +impl ProtocolHub { + pub fn heartbeat( + &self, + connection_id: u64, + key: &AssignmentKey, + lease_id: &str, + now_ms: i64, + ) -> Result<(i64, i64)> { + let mut sessions = self.sessions.lock().expect("protocol sessions lock"); + let assignment_deadline = { + let assignment = sessions + .assignments + .get_mut(key) + .context("attempt has no active worker lease")?; + if assignment.connection_id != connection_id || assignment.lease_id != lease_id { + bail!("heartbeat does not match the active worker lease") + } + assignment.lease_deadline_ms = now_ms.saturating_add(LEASE_RENEWAL_MS); + assignment.lease_deadline_ms + }; + let run_deadline = sessions + .assignments + .iter() + .filter(|((run_id, _, _), _)| run_id == &key.0) + .map(|(_, assignment)| assignment.lease_deadline_ms) + .min() + .expect("the renewed assignment is still present"); + Ok((assignment_deadline, run_deadline)) + } + + pub fn completion_worker(&self, connection_id: u64, key: &AssignmentKey) -> Result { + let sessions = self.sessions.lock().expect("protocol sessions lock"); + let assignment = sessions + .assignments + .get(key) + .context("attempt has no active worker lease")?; + if assignment.connection_id != connection_id { + bail!("completion came from a worker that does not hold the lease") + } + Ok(assignment.worker_id.clone()) + } + + pub fn finish(&self, key: &AssignmentKey) { + self.sessions + .lock() + .expect("protocol sessions lock") + .assignments + .remove(key); + } + + /// Release every in-memory lease after the journal has durably made the + /// run terminal. Calling this repeatedly is intentionally harmless. Under + /// parallel dispatch a terminal run can hold several live leases at once, + /// so this releases the whole run rather than one attempt. + pub fn finish_run(&self, run_id: &str) { + self.sessions + .lock() + .expect("protocol sessions lock") + .assignments + .retain(|(assigned_run, _, _), _| assigned_run != run_id); + } + + pub fn earliest_lease_deadline(&self, run_id: &str) -> Option { + let sessions = self.sessions.lock().expect("protocol sessions lock"); + sessions + .assignments + .iter() + .filter(|((assigned_run, _, _), assignment)| { + assigned_run == run_id + && sessions + .workers + .iter() + .any(|worker| worker.connection_id == assignment.connection_id) + }) + .map(|(_, assignment)| assignment.lease_deadline_ms) + .min() + } + + pub fn expired_assignments(&self, now_ms: i64) -> Vec { + self.sessions + .lock() + .expect("protocol sessions lock") + .assignments + .iter() + .filter(|(_, assignment)| now_ms >= assignment.lease_deadline_ms) + .map(|((run_id, step_id, attempt), _)| AbandonedLease { + run_id: run_id.clone(), + step_id: step_id.clone(), + attempt: *attempt, + }) + .collect() + } +} + +impl StepDispatcher for ProtocolHub { + fn executor(&self, step_type: StepType) -> Option { + let sessions = self.sessions.lock().expect("protocol sessions lock"); + select_worker(&sessions, step_type).map(|worker| worker.worker_id.clone()) + } + + fn available(&self, step_type: StepType) -> bool { + self.executor(step_type).is_some() + } + + fn reserve_dispatch( + &self, + run_id: &str, + step: &StepSpec, + attempt: u32, + required_pins: &Pins, + ) -> Result { + let mut sessions = self.sessions.lock().expect("protocol sessions lock"); + let key = (run_id.to_owned(), step.id.clone(), attempt); + if sessions.reservations.contains_key(&key) { + return Ok(true); + } + let Some(connection_id) = select_worker_for_step(&sessions, step, required_pins) + .map(|worker| worker.connection_id) + else { + return Ok(false); + }; + sessions + .reservations + .insert(key, Reservation { connection_id }); + Ok(true) + } + + fn reserved_executor(&self, run_id: &str, step: &StepSpec, attempt: u32) -> Option { + let sessions = self.sessions.lock().expect("protocol sessions lock"); + let reservation = + sessions + .reservations + .get(&(run_id.to_owned(), step.id.clone(), attempt))?; + sessions + .workers + .iter() + .find(|worker| worker.connection_id == reservation.connection_id) + .map(|worker| worker.worker_id.clone()) + } + + fn reserved_starting_pins(&self, run_id: &str, step: &StepSpec, attempt: u32) -> Result { + let sessions = self.sessions.lock().expect("protocol sessions lock"); + let reservation = sessions + .reservations + .get(&(run_id.to_owned(), step.id.clone(), attempt)) + .context("dispatch has no worker reservation")?; + let worker = sessions + .workers + .iter() + .find(|worker| worker.connection_id == reservation.connection_id) + .context("reserved worker detached before start pins were journaled")?; + let StepKind::Agent { surfaces, .. } = &step.kind else { + return Ok(Pins::default()); + }; + let workspace = surfaces + .workspace + .iter() + .map(|surface| { + worker + .pins + .workspace + .iter() + .find(|pin| pin.surface == surface.surface) + .cloned() + .with_context(|| { + format!( + "worker {} omitted revision for surface {}", + worker.worker_id, surface.surface + ) + }) + }) + .collect::>>()?; + let streams = surfaces + .streams + .iter() + .map(|surface| { + worker + .pins + .streams + .iter() + .find(|pin| pin.stream == surface.stream) + .cloned() + .with_context(|| { + format!( + "worker {} omitted read offset for stream {}", + worker.worker_id, surface.stream + ) + }) + }) + .collect::>>()?; + Ok(Pins { workspace, streams }) + } + + fn release_dispatch_reservation(&self, run_id: &str, step_id: &str, attempt: u32) { + self.sessions + .lock() + .expect("protocol sessions lock") + .reservations + .remove(&(run_id.to_owned(), step_id.to_owned(), attempt)); + } + + fn dispatch(&self, dispatch: StepDispatch) -> Result { + let mut sessions = self.sessions.lock().expect("protocol sessions lock"); + let key = ( + dispatch.run_id.clone(), + dispatch.step_id.clone(), + dispatch.attempt, + ); + let Some(connection_id) = sessions + .reservations + .get(&key) + .map(|reservation| reservation.connection_id) + else { + return Ok(DispatchOutcome::NoWorker); + }; + let Some(worker) = sessions + .workers + .iter() + .find(|worker| worker.connection_id == connection_id) + .cloned() + else { + sessions.reservations.remove(&key); + return Ok(DispatchOutcome::NoWorker); + }; + if !worker_holds(&worker, &dispatch.pins) { + sessions.reservations.remove(&key); + return Ok(DispatchOutcome::NoWorker); + } + if let Some(detail) = pin_value_mismatch(&worker, &dispatch) { + sessions.reservations.remove(&key); + return Ok(DispatchOutcome::PinMismatch { detail }); + } + if let Err(error) = write_frame( + &worker.writer, + &json!({"event": "step.dispatch", "data": dispatch}), + ) + .with_context(|| format!("dispatch step to worker {}", worker.worker_id)) + { + sessions.reservations.remove(&key); + return Err(error); + } + sessions.reservations.remove(&key); + sessions.assignments.insert( + key, + Assignment { + connection_id: worker.connection_id, + worker_id: worker.worker_id, + lease_id: dispatch.lease_id, + lease_deadline_ms: dispatch.lease_deadline_ms, + }, + ); + Ok(DispatchOutcome::Dispatched) + } + + fn active_lease_deadline(&self, run_id: &str, step_id: &str, attempt: u32) -> Option { + let sessions = self.sessions.lock().expect("protocol sessions lock"); + let assignment = + sessions + .assignments + .get(&(run_id.to_owned(), step_id.to_owned(), attempt))?; + sessions + .workers + .iter() + .any(|worker| worker.connection_id == assignment.connection_id) + .then_some(assignment.lease_deadline_ms) + } +} + +impl LeaseProbe for ProtocolHub { + fn lease_active(&self, run_id: &str, step_id: &str, attempt: u32, now_ms: i64) -> bool { + let key = (run_id.to_owned(), step_id.to_owned(), attempt); + let sessions = self.sessions.lock().expect("protocol sessions lock"); + sessions.assignments.get(&key).is_some_and(|assignment| { + now_ms < assignment.lease_deadline_ms + && sessions + .workers + .iter() + .any(|worker| worker.connection_id == assignment.connection_id) + }) + } +} diff --git a/kernel/relayflowd/src/server/session/matching.rs b/kernel/relayflowd/src/server/session/matching.rs index 22d807a5f..e36cf8ad9 100644 --- a/kernel/relayflowd/src/server/session/matching.rs +++ b/kernel/relayflowd/src/server/session/matching.rs @@ -2,17 +2,93 @@ //! the attempt is pinned. Split from `session.rs` so worker selection and //! Appendix A rule 2's starting-state check read as their own subject. -use relayflowd_core::{Pins, StepType}; +use std::cmp::Ordering; + +use relayflowd_core::{Pins, StepKind, StepSpec, StepType}; use super::{Sessions, Worker}; use crate::worker::StepDispatch; -/// The single worker-selection rule, shared by pin sourcing and dispatch. +/// Deterministic least-loaded selection. Capacity is counted across both +/// journal-pending reservations and live assignments, so admission happens +/// before a durable start and concurrent runs cannot overbook a worker. pub(super) fn select_worker(sessions: &Sessions, step_type: StepType) -> Option<&Worker> { sessions .workers .iter() - .find(|worker| worker.step_types.contains(&step_type)) + .filter(|worker| worker.step_types.contains(&step_type)) + .filter_map(|worker| { + let load = worker_load(sessions, worker.connection_id); + (load < worker.capacity).then_some((worker, load)) + }) + .min_by(|(left, left_load), (right, right_load)| { + normalized_load_order(*left_load, left.capacity, *right_load, right.capacity) + }) + .map(|(worker, _)| worker) +} + +pub(super) fn select_worker_for_step<'a>( + sessions: &'a Sessions, + step: &StepSpec, + required_pins: &Pins, +) -> Option<&'a Worker> { + sessions + .workers + .iter() + .filter(|worker| worker.step_types.contains(&step.step_type())) + .filter(|worker| worker_can_pin(worker, step, required_pins)) + .filter_map(|worker| { + let load = worker_load(sessions, worker.connection_id); + (load < worker.capacity).then_some((worker, load)) + }) + .min_by(|(left, left_load), (right, right_load)| { + normalized_load_order(*left_load, left.capacity, *right_load, right.capacity) + }) + .map(|(worker, _)| worker) +} + +fn worker_load(sessions: &Sessions, connection_id: u64) -> usize { + sessions + .assignments + .values() + .filter(|assignment| assignment.connection_id == connection_id) + .count() + + sessions + .reservations + .values() + .filter(|reservation| reservation.connection_id == connection_id) + .count() +} + +fn normalized_load_order( + left_load: usize, + left_capacity: usize, + right_load: usize, + right_capacity: usize, +) -> Ordering { + (left_load as u128 * right_capacity as u128).cmp(&(right_load as u128 * left_capacity as u128)) +} + +fn worker_can_pin(worker: &Worker, step: &StepSpec, required_pins: &Pins) -> bool { + if !worker_holds(worker, required_pins) { + return false; + } + let StepKind::Agent { surfaces, .. } = &step.kind else { + return true; + }; + surfaces.workspace.iter().all(|declared| { + worker + .pins + .workspace + .iter() + .any(|held| held.surface == declared.surface) + }) && surfaces.streams.iter().all(|declared| { + worker + .pins + .streams + .iter() + .any(|held| held.stream == declared.stream) + }) } /// Which pinned values does the selected worker not stand at? `None` when the diff --git a/kernel/relayflowd/src/server/tests.rs b/kernel/relayflowd/src/server/tests.rs index 86a10af60..ff67796a9 100644 --- a/kernel/relayflowd/src/server/tests.rs +++ b/kernel/relayflowd/src/server/tests.rs @@ -249,7 +249,7 @@ fn an_entry_appended_during_watch_registration_is_delivered_exactly_once() { let (control_writer, _control_peer) = shared_writer(); let spec = json!({ "name": "watch-gap", - "steps": [{"id": "only", "type": "deterministic", "command": "true"}] + "steps": [{"id": "only", "type": "llm", "prompt": "wait for a worker"}] }); let line = json!({"id": "start", "verb": "run.start", "params": {"spec": spec}}).to_string(); let started = request(data_dir, &hub, 2, &control_writer, &line); @@ -281,12 +281,7 @@ fn an_entry_appended_during_watch_registration_is_delivered_exactly_once() { let append = thread::spawn(move || { let _guard = append_lock.lock().unwrap(); interleaver - .append_stream( - &append_run, - "results", - "test", - json!({"interleaved": true}), - ) + .append_stream(&append_run, "results", "test", json!({"interleaved": true})) .unwrap(); }); wait_for_signal(&committed); diff --git a/kernel/relayflowd/src/server/wire.rs b/kernel/relayflowd/src/server/wire.rs index 1f165245a..e61bedfe2 100644 --- a/kernel/relayflowd/src/server/wire.rs +++ b/kernel/relayflowd/src/server/wire.rs @@ -52,10 +52,16 @@ pub(super) struct RunIdParams { pub(super) struct WorkerAttachParams { pub worker_id: String, pub step_types: Vec, + #[serde(default = "default_worker_capacity")] + pub capacity: usize, #[serde(default)] pub pins: Pins, } +fn default_worker_capacity() -> usize { + 1 +} + #[derive(Deserialize)] #[serde(deny_unknown_fields)] pub(super) struct StepHeartbeatParams { diff --git a/kernel/relayflowd/src/worker.rs b/kernel/relayflowd/src/worker.rs index 8a8bf19d4..b64fdb323 100644 --- a/kernel/relayflowd/src/worker.rs +++ b/kernel/relayflowd/src/worker.rs @@ -44,12 +44,43 @@ pub trait StepDispatcher: Send + Sync { fn available(&self, step_type: StepType) -> bool; + /// Reserve one worker slot before the durable start is appended. Runtime + /// dispatchers override this atomically; stateless/in-process dispatchers + /// retain the legacy availability behavior through this default. + fn reserve_dispatch( + &self, + _run_id: &str, + step: &StepSpec, + _attempt: u32, + _required_pins: &Pins, + ) -> Result { + Ok(self.available(step.step_type())) + } + + /// Executor selected by [`reserve_dispatch`](Self::reserve_dispatch). + fn reserved_executor(&self, _run_id: &str, step: &StepSpec, _attempt: u32) -> Option { + self.executor(step.step_type()) + } + /// Opaque starting revisions/offsets reported by the selected worker for /// the first agent attempt. Later attempts are derived from the journal. fn starting_pins(&self, _step: &StepSpec) -> Result { Ok(Pins::default()) } + /// Starting pins reported by the worker whose capacity was reserved. + fn reserved_starting_pins( + &self, + _run_id: &str, + step: &StepSpec, + _attempt: u32, + ) -> Result { + self.starting_pins(step) + } + + /// Release an admission that did not become a live assignment. + fn release_dispatch_reservation(&self, _run_id: &str, _step_id: &str, _attempt: u32) {} + fn dispatch(&self, dispatch: StepDispatch) -> Result; /// Heartbeat-renewed operational deadline for one live assignment. The diff --git a/kernel/relayflowd/tests/crash_resume.rs b/kernel/relayflowd/tests/crash_resume.rs index 51e5b6014..d06abdbe7 100644 --- a/kernel/relayflowd/tests/crash_resume.rs +++ b/kernel/relayflowd/tests/crash_resume.rs @@ -13,8 +13,14 @@ mod llm; mod llm_support; #[path = "crash_resume/parallel_lifecycle.rs"] mod parallel_lifecycle; +#[path = "crash_resume/protocol_admission.rs"] +mod protocol_admission; #[path = "crash_resume/support.rs"] mod support; +#[path = "crash_resume/surface_identity.rs"] +mod surface_identity; +#[path = "crash_resume/worker_capacity.rs"] +mod worker_capacity; use std::{ fs, io::Write, os::unix::net::UnixStream, os::unix::process::CommandExt, process::Command, diff --git a/kernel/relayflowd/tests/crash_resume/llm_support.rs b/kernel/relayflowd/tests/crash_resume/llm_support.rs index 1fa104bfc..693633a16 100644 --- a/kernel/relayflowd/tests/crash_resume/llm_support.rs +++ b/kernel/relayflowd/tests/crash_resume/llm_support.rs @@ -123,6 +123,20 @@ impl LlmFixture { fixture } + pub fn completed(name: &str) -> Self { + let mut fixture = Self::new(name, false); + fixture.spec = json!({ + "name": format!("completed-{name}"), + "steps": [{"id": "done", "type": "deterministic", "command": "true"}] + }); + fs::write( + &fixture.spec_path, + serde_json::to_vec(&fixture.spec).unwrap(), + ) + .unwrap(); + fixture + } + pub fn parallel_terminal(name: &str) -> Self { let mut fixture = Self::parallel(name); for step in fixture.spec["steps"].as_array_mut().unwrap() { @@ -236,6 +250,26 @@ impl ProtocolClient { } pub fn request(&mut self, verb: &str, params: Value) -> Result { + let frame = self.request_frame(verb, params)?; + if frame["ok"] == true { + return Ok(frame.get("result").cloned().unwrap_or(Value::Null)); + } + bail!( + "{}: {}", + frame["error"]["code"].as_str().unwrap_or("protocol_error"), + frame["error"]["message"] + .as_str() + .unwrap_or("missing detail") + ) + } + + pub fn request_error_code(&mut self, verb: &str, params: Value) -> String { + let frame = self.request_frame(verb, params).unwrap(); + assert_eq!(frame["ok"], false, "{verb} unexpectedly succeeded"); + frame["error"]["code"].as_str().unwrap().to_owned() + } + + fn request_frame(&mut self, verb: &str, params: Value) -> Result { let id = format!("test-{}", self.next_id); self.next_id += 1; serde_json::to_writer( @@ -253,16 +287,7 @@ impl ProtocolClient { if frame["id"] != id { continue; } - if frame["ok"] == true { - return Ok(frame.get("result").cloned().unwrap_or(Value::Null)); - } - bail!( - "{}: {}", - frame["error"]["code"].as_str().unwrap_or("protocol_error"), - frame["error"]["message"] - .as_str() - .unwrap_or("missing detail") - ); + return Ok(frame); } } @@ -299,7 +324,7 @@ pub fn attached_worker(fixture: &LlmFixture, id: &str) -> ProtocolClient { worker .request( "worker.attach", - json!({"worker_id": id, "step_types": ["llm"]}), + json!({"worker_id": id, "step_types": ["llm"], "capacity": 8}), ) .unwrap(); worker diff --git a/kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs b/kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs index 702147e82..26275928a 100644 --- a/kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs +++ b/kernel/relayflowd/tests/crash_resume/parallel_lifecycle.rs @@ -23,6 +23,7 @@ fn attached_agent(fixture: &LlmFixture, id: &str) -> ProtocolClient { json!({ "worker_id": id, "step_types": ["agent"], + "capacity": 8, "pins": {"workspace": [ {"surface": "repo-b", "revision_id": "r0"}, {"surface": "repo-a", "revision_id": "r0"} diff --git a/kernel/relayflowd/tests/crash_resume/protocol_admission.rs b/kernel/relayflowd/tests/crash_resume/protocol_admission.rs new file mode 100644 index 000000000..a136fe614 --- /dev/null +++ b/kernel/relayflowd/tests/crash_resume/protocol_admission.rs @@ -0,0 +1,83 @@ +//! Terminality is enforced at the live protocol mutation boundary. + +use relayflowd_core::EntryType; +use serde_json::json; + +use super::{ + llm_support::{LlmFixture, ProtocolClient, ServerGuard}, + support::journal_entries, +}; + +#[test] +fn every_mutating_run_verb_refuses_terminal_before_changing_state() { + let fixture = LlmFixture::completed("terminal-admission"); + let _server = ServerGuard::start(&fixture); + let socket = fixture.data_dir.join("relayflowd.sock"); + let mut client = ProtocolClient::connect(&socket); + let started = client + .request( + "run.start", + json!({"spec": { + "steps": [{"id": "done", "type": "deterministic", "command": "true"}] + }}), + ) + .unwrap(); + assert_eq!(started["status"], "completed"); + let run_id = started["run_id"].as_str().unwrap(); + let before = journal_entries(&fixture.data_dir).unwrap(); + assert_eq!(before.last().unwrap().entry_type, EntryType::RunCompleted); + + let cases = [ + ( + "stream.append", + json!({"run_id": run_id, "stream": "late", "message": {"bad": true}}), + ), + ( + "event.emit", + json!({"run_id": run_id, "event_key": "late", "payload": {}}), + ), + ( + "effect.record", + json!({ + "run_id": run_id, "step_id": "done", "attempt": 1, + "idempotency_key": "late", "surface_path": "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/provider/item", + "revision_before": "a", "revision_after": "b" + }), + ), + ( + "effect.confirm", + json!({ + "run_id": run_id, "step_id": "done", "attempt": 1, + "idempotency_key": "late", "surface_path": "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/provider/item" + }), + ), + ( + "step.complete", + json!({ + "run_id": run_id, "step_id": "done", "attempt": 1, + "idempotency_key": "late", "completionReason": "success" + }), + ), + ( + "step.heartbeat", + json!({ + "run_id": run_id, "step_id": "done", "attempt": 1, + "lease_id": "late" + }), + ), + ]; + for (verb, params) in cases { + assert_eq!(client.request_error_code(verb, params), "run_terminal"); + assert_eq!( + journal_entries(&fixture.data_dir).unwrap().len(), + before.len(), + "{verb} changed the terminal journal" + ); + } + assert_eq!( + client + .request("run.get", json!({"run_id": run_id})) + .unwrap()["status"], + "completed" + ); +} diff --git a/kernel/relayflowd/tests/crash_resume/surface_identity.rs b/kernel/relayflowd/tests/crash_resume/surface_identity.rs new file mode 100644 index 000000000..168a3fbac --- /dev/null +++ b/kernel/relayflowd/tests/crash_resume/surface_identity.rs @@ -0,0 +1,111 @@ +//! External mount-write identity is canonical and subtree-aware. + +use relayflowd_core::EntryType; +use serde_json::{Value, json}; + +use super::{ + llm_support::{LlmFixture, ProtocolClient, ServerGuard}, + support::journal_entries, +}; + +fn complete(worker: &mut ProtocolClient, dispatch: &Value) -> Value { + worker + .request( + "step.complete", + json!({ + "run_id": dispatch["run_id"], + "step_id": dispatch["step_id"], + "attempt": dispatch["attempt"], + "idempotency_key": dispatch["idempotency_key"], + "completionReason": "success", + "output": {"done": dispatch["step_id"]}, + "started_pins": dispatch["pins"], + "end_pins": {} + }), + ) + .unwrap() +} + +#[test] +fn aliases_are_rejected_and_external_ancestors_serialize_over_real_sockets() { + let fixture = LlmFixture::parallel("surface-identity"); + let _server = ServerGuard::start(&fixture); + let socket = fixture.data_dir.join("relayflowd.sock"); + let mut worker = ProtocolClient::connect(&socket); + worker + .request( + "worker.attach", + json!({ + "worker_id": "surface-worker", + "step_types": ["agent"], + "capacity": 2, + "pins": {"workspace": [{"surface": "unused", "revision_id": "r0"}]} + }), + ) + .unwrap(); + let mut control = ProtocolClient::connect(&socket); + for alias in [ + "/provider/./item", + "/provider/../item", + "/provider//item", + "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/provider/item/", + ] { + assert_eq!( + control.request_error_code( + "run.start", + json!({"spec": {"steps": [{ + "id": "bad", "type": "agent", "instruction": "bad", + "surfaces": {"external": [alias]} + }]}}), + ), + "invalid_spec" + ); + } + + let started = control + .request( + "run.start", + json!({"spec": {"steps": [ + {"id": "parent", "type": "agent", "instruction": "parent", + "surfaces": {"external": ["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/provider/item"]}}, + {"id": "child", "type": "agent", "instruction": "child", + "surfaces": {"external": ["/provider/item/child"]}} + ]}}), + ) + .unwrap(); + let parent = worker.event("step.dispatch").unwrap(); + assert_eq!(parent["step_id"], "parent"); + assert_eq!( + journal_entries(&fixture.data_dir) + .unwrap() + .iter() + .filter(|entry| { + entry.run_id == started["run_id"] + && entry.entry_type == EntryType::StepAttemptStarted + }) + .count(), + 1 + ); + complete(&mut worker, &parent); + let child = worker.event("step.dispatch").unwrap(); + assert_eq!(child["step_id"], "child"); + assert_eq!(complete(&mut worker, &child)["status"], "completed"); + + let disjoint = control + .request( + "run.start", + json!({"spec": {"steps": [ + {"id": "left", "type": "agent", "instruction": "left", + "surfaces": {"external": ["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/provider/left"]}}, + {"id": "right", "type": "agent", "instruction": "right", + "surfaces": {"external": ["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/provider/right"]}} + ]}}), + ) + .unwrap(); + assert_eq!(disjoint["status"], "parked"); + let left = worker.event("step.dispatch").unwrap(); + let right = worker.event("step.dispatch").unwrap(); + assert_eq!([&left["step_id"], &right["step_id"]], ["left", "right"]); + complete(&mut worker, &right); + assert_eq!(complete(&mut worker, &left)["status"], "completed"); +} diff --git a/kernel/relayflowd/tests/crash_resume/worker_capacity.rs b/kernel/relayflowd/tests/crash_resume/worker_capacity.rs new file mode 100644 index 000000000..8eafc8580 --- /dev/null +++ b/kernel/relayflowd/tests/crash_resume/worker_capacity.rs @@ -0,0 +1,158 @@ +//! Real placement, capacity release, and crash recovery at the socket boundary. + +use relayflowd_core::{CompletionReason, EntryType, StepCompletedPayload}; +use serde_json::json; + +use super::{ + llm_support::{LlmFixture, ProtocolClient, ServerGuard, complete}, + support::{journal_entries, wait_until}, +}; + +fn attach(fixture: &LlmFixture, worker_id: &str, capacity: Option) -> ProtocolClient { + let mut worker = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + let mut params = json!({"worker_id": worker_id, "step_types": ["llm"]}); + if let Some(capacity) = capacity { + params["capacity"] = json!(capacity); + } + worker.request("worker.attach", params).unwrap(); + worker +} + +#[test] +fn two_workers_receive_a_deterministic_fair_capacity_bounded_batch() { + let fixture = LlmFixture::parallel("fair-capacity"); + let _server = ServerGuard::start(&fixture); + let mut first = attach(&fixture, "first", Some(2)); + let mut second = attach(&fixture, "second", Some(2)); + let mut control = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + let started = control + .request( + "run.start", + json!({"spec": {"steps": [ + {"id": "lane-1", "type": "llm", "prompt": "1", "model": "stub"}, + {"id": "lane-2", "type": "llm", "prompt": "2", "model": "stub"}, + {"id": "lane-3", "type": "llm", "prompt": "3", "model": "stub"}, + {"id": "lane-4", "type": "llm", "prompt": "4", "model": "stub"} + ]}}), + ) + .unwrap(); + assert_eq!(started["status"], "parked"); + let first_dispatches = [ + first.event("step.dispatch").unwrap(), + first.event("step.dispatch").unwrap(), + ]; + let second_dispatches = [ + second.event("step.dispatch").unwrap(), + second.event("step.dispatch").unwrap(), + ]; + assert_eq!( + first_dispatches + .iter() + .map(|dispatch| dispatch["step_id"].as_str().unwrap()) + .collect::>(), + ["lane-1", "lane-3"] + ); + assert_eq!( + second_dispatches + .iter() + .map(|dispatch| dispatch["step_id"].as_str().unwrap()) + .collect::>(), + ["lane-2", "lane-4"] + ); + for dispatch in &first_dispatches { + complete(&mut first, dispatch, json!({"done": dispatch["step_id"]})).unwrap(); + } + for dispatch in &second_dispatches { + complete(&mut second, dispatch, json!({"done": dispatch["step_id"]})).unwrap(); + } + assert_eq!( + control + .request("run.get", json!({"run_id": started["run_id"]})) + .unwrap()["status"], + "completed" + ); +} + +#[test] +fn default_capacity_one_reopens_only_after_durable_completion_or_crash() { + let fixture = LlmFixture::parallel("capacity-one-completion"); + let _server = ServerGuard::start(&fixture); + let mut worker = attach(&fixture, "serial", None); + let mut control = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + let started = control + .request( + "run.start", + json!({"spec": { + "steps": [ + {"id": "lane-b", "type": "llm", "prompt": "b", "model": "stub"}, + {"id": "lane-a", "type": "llm", "prompt": "a", "model": "stub"} + ] + }}), + ) + .unwrap(); + let lane_b = worker.event("step.dispatch").unwrap(); + assert_eq!(lane_b["step_id"], "lane-b"); + assert_eq!( + journal_entries(&fixture.data_dir) + .unwrap() + .iter() + .filter(|entry| entry.entry_type == EntryType::StepAttemptStarted) + .count(), + 1, + "capacity must be reserved before another start is journaled" + ); + complete(&mut worker, &lane_b, json!({"done": "b"})).unwrap(); + let lane_a = worker.event("step.dispatch").unwrap(); + assert_eq!(lane_a["step_id"], "lane-a"); + assert_eq!( + complete(&mut worker, &lane_a, json!({"done": "a"})).unwrap()["status"], + "completed" + ); + assert_eq!( + control + .request("run.get", json!({"run_id": started["run_id"]})) + .unwrap()["status"], + "completed" + ); + + let fixture = LlmFixture::parallel("capacity-one-crash"); + let _server = ServerGuard::start(&fixture); + let mut crashed = attach(&fixture, "crashed", None); + let mut control = ProtocolClient::connect(&fixture.data_dir.join("relayflowd.sock")); + let started = control + .request( + "run.start", + json!({"spec": { + "steps": [ + {"id": "lane-b", "type": "llm", "prompt": "b", "model": "stub"}, + {"id": "lane-a", "type": "llm", "prompt": "a", "model": "stub"} + ] + }}), + ) + .unwrap(); + assert_eq!(crashed.event("step.dispatch").unwrap()["step_id"], "lane-b"); + drop(crashed); + wait_until("durable crashed completion", || { + journal_entries(&fixture.data_dir).is_some_and(|entries| { + entries.iter().any(|entry| { + entry.entry_type == EntryType::StepCompleted + && serde_json::from_value::(entry.payload.clone()) + .is_ok_and(|payload| payload.completion_reason == CompletionReason::Crashed) + }) + }) + }); + let mut replacement = attach(&fixture, "replacement", None); + control + .request("run.resume", json!({"run_id": started["run_id"]})) + .unwrap(); + let retried = replacement.event("step.dispatch").unwrap(); + assert_eq!(retried["step_id"], "lane-b"); + assert_eq!(retried["attempt"], 2); + complete(&mut replacement, &retried, json!({"done": "b"})).unwrap(); + let sibling = replacement.event("step.dispatch").unwrap(); + assert_eq!(sibling["step_id"], "lane-a"); + assert_eq!( + complete(&mut replacement, &sibling, json!({"done": "a"})).unwrap()["status"], + "completed" + ); +} diff --git a/sdk/src/journal-client.ts b/sdk/src/journal-client.ts index b26e73f1a..eb26f2a06 100644 --- a/sdk/src/journal-client.ts +++ b/sdk/src/journal-client.ts @@ -203,8 +203,8 @@ export class JournalClient extends EventEmitter { } /** Connection becomes a worker; receives `step.dispatch` events. */ - workerAttach(workerId: string, stepTypes: StepType[], pins?: Pins): Promise { - return this.request('worker.attach', { worker_id: workerId, step_types: stepTypes, pins }); + workerAttach(workerId: string, stepTypes: StepType[], pins?: Pins, capacity?: number): Promise { + return this.request('worker.attach', { worker_id: workerId, step_types: stepTypes, pins, capacity }); } /** diff --git a/sdk/src/protocol.ts b/sdk/src/protocol.ts index 3ec7f3063..1bb5a01e4 100644 --- a/sdk/src/protocol.ts +++ b/sdk/src/protocol.ts @@ -133,6 +133,8 @@ export interface RunWatchResult { export interface WorkerAttachParams { worker_id: string; step_types: StepType[]; + /** Maximum concurrent assignments. Omitted means the conservative default 1. */ + capacity?: number; /** * The surfaces this worker holds, as opaque revisions/offsets. Required when * `step_types` includes `agent` — an agent attempt's start pins come from diff --git a/sdk/src/validate.ts b/sdk/src/validate.ts index ec0b75b59..053f814b5 100644 --- a/sdk/src/validate.ts +++ b/sdk/src/validate.ts @@ -47,6 +47,21 @@ const RECOVERY_MODES: ReadonlySet = new Set([ const DECIMAL_RE = /^\d+(\.\d+)?$/; +function isCanonicalExternalSurface(value: unknown): value is string { + if (!isNonEmptyString(value) || value.trim() !== value) return false; + let tail = value; + if (tail.startsWith('/')) tail = tail.slice(1); + else { + const scheme = tail.indexOf('://'); + if (scheme >= 0) { + if (scheme === 0 || tail.slice(0, scheme).includes('/')) return false; + tail = tail.slice(scheme + 3); + } + } + if (tail === '') return true; + return tail.split('/').every((part) => part !== '' && part !== '.' && part !== '..'); +} + // Allowed keys per authoring object level. Validation is fail-closed on // unknown keys (AGENTS.md rule 4; RFC covenant 2): a typo'd key like // `depends_on` must be an error naming the nearest valid key, never a @@ -376,8 +391,8 @@ class Validator { } } if (s['external'] !== undefined) { - if (!Array.isArray(s['external']) || !(s['external'] as unknown[]).every(isNonEmptyString)) { - this.fail(`${at}.external: expected an array of path strings`); + if (!Array.isArray(s['external']) || !(s['external'] as unknown[]).every(isCanonicalExternalSurface)) { + this.fail(`${at}.external: expected canonical path strings without empty, . or .. components`); } } } diff --git a/sdk/src/worker.ts b/sdk/src/worker.ts index 6dece42c9..22088d11b 100644 --- a/sdk/src/worker.ts +++ b/sdk/src/worker.ts @@ -9,6 +9,7 @@ export { MODEL_ENV, WAKE_CONTEXT_ENV } from './worker-cli.js'; export interface AgentWorkerOptions { workerId: string; pins: Pins; + capacity?: number; } /** @@ -44,7 +45,12 @@ export class AgentWorker extends EventEmitter { if (this.closing) throw new Error('agent worker: cannot attach a closed worker (construct a new one)'); this.client.on('step.dispatch', this.onDispatch); try { - await this.client.workerAttach(this.options.workerId, ['agent'], this.options.pins); + await this.client.workerAttach( + this.options.workerId, + ['agent'], + this.options.pins, + this.options.capacity, + ); this.attached = true; } catch (error) { this.client.off('step.dispatch', this.onDispatch); diff --git a/sdk/tests/validate.test.ts b/sdk/tests/validate.test.ts index 070093b84..6c2e0ec64 100644 --- a/sdk/tests/validate.test.ts +++ b/sdk/tests/validate.test.ts @@ -299,6 +299,37 @@ describe('validate: accepts the legal zero-agent flow', () => { }); }); +describe('validate: canonical external surfaces', () => { + it.each([ + '/provider/./item', + '/provider/../item', + '/provider//item', + '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/provider/item/', + ])('rejects alias %s', (external) => { + const result = validateSpec({ + version: '0.1.0', + steps: [{ + id: 'agent', type: 'agent', instruction: 'write', + surfaces: { external: [external] }, + }], + }); + expect(result.ok).toBe(false); + expect(result.errors.join(' ')).toContain('canonical path strings'); + }); + + it('accepts canonical absolute, relative, and URI-like identities', () => { + for (const external of ['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/provider/item', 'provider/item', 'pr://github/example']) { + expect(validateSpec({ + version: '0.1.0', + steps: [{ + id: 'agent', type: 'agent', instruction: 'write', + surfaces: { external: [external] }, + }], + })).toEqual({ ok: true, errors: [] }); + } + }); +}); + describe('validate: preflight declarations', () => { it('accepts CLI defaults and inert trigger data', () => { const result = validateSpec({ From 8edba0653768137ed33374083f46af1d186dac95 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 22:18:36 +0200 Subject: [PATCH 05/14] fix(kernel): reject forged completion pins Session-Id: 01a062cc-f525-7d01-932e-a634815114c1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd/src/engine/remote.rs | 12 ++- kernel/relayflowd/src/server.rs | 7 -- kernel/relayflowd/src/server/session.rs | 29 +++--- kernel/relayflowd/tests/crash_resume.rs | 2 + .../tests/crash_resume/pin_projection.rs | 88 +++++++++++++++++++ 5 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 kernel/relayflowd/tests/crash_resume/pin_projection.rs diff --git a/kernel/relayflowd/src/engine/remote.rs b/kernel/relayflowd/src/engine/remote.rs index 9b7ca6358..de1b4a8b6 100644 --- a/kernel/relayflowd/src/engine/remote.rs +++ b/kernel/relayflowd/src/engine/remote.rs @@ -66,7 +66,9 @@ impl Engine { // nulled for every non-success, so the detail rides the completion's // verification record — the same channel a failed gate uses. let mut failure_detail = None; + let mut rejected_completion = false; let mut reject = |error: anyhow::Error| { + rejected_completion = true; failure_reason = Some(CompletionReason::WorkerError); failure_detail = Some(format!("{error:#}")); }; @@ -92,11 +94,19 @@ impl Engine { )); Vec::new() }; + // Rejected evidence cannot become authoritative state. In particular, + // inspect retries must inherit the last accepted pins, not an end pin + // carried by a completion whose claimed starting point was invalid. + let end_pins = if matches!(step.kind, StepKind::Agent { .. }) && rejected_completion { + None + } else { + completion.end_pins + }; let result = AttemptResult { output: completion.output, budget: completion.budget, completed_by: completion.completed_by, - end_pins: completion.end_pins, + end_pins, effects, trajectory_tail: completion.trajectory_tail, failure_reason, diff --git a/kernel/relayflowd/src/server.rs b/kernel/relayflowd/src/server.rs index c827bb9da..67bef6740 100644 --- a/kernel/relayflowd/src/server.rs +++ b/kernel/relayflowd/src/server.rs @@ -270,13 +270,6 @@ fn handle_request( let worker_id = hub .completion_worker(connection_id, &key) .map_err(protocol_conflict)?; - // The worker moved the surfaces its completion pins; the hub's view - // of what it holds moves with it *before* the completion drives the - // run, so the next attempt's chained pins are checked against the - // worker's real state rather than its attach snapshot. - if let Some(end_pins) = ¶ms.end_pins { - hub.advance_worker_pins(connection_id, end_pins); - } let outcome = engine .complete_out_of_band( ¶ms.run_id, diff --git a/kernel/relayflowd/src/server/session.rs b/kernel/relayflowd/src/server/session.rs index 9ac43d93b..324952c32 100644 --- a/kernel/relayflowd/src/server/session.rs +++ b/kernel/relayflowd/src/server/session.rs @@ -6,7 +6,9 @@ use std::{ }; use anyhow::Result; -use relayflowd_core::{CompletionReason, JournalEntry, Pins, StepType}; +use relayflowd_core::{ + CompletionReason, EntryType, JournalEntry, Pins, StepCompletedPayload, StepType, +}; use serde_json::json; use crate::worker::JournalObserver; @@ -129,13 +131,10 @@ impl ProtocolHub { }); } - /// A worker's advertised state is only true until it changes it. A step it - /// completes moves the surfaces that completion pins, so the hub's view - /// must move with it — otherwise the next attempt's pins, chained from - /// those very end pins, would read as a mismatch against a stale snapshot. - /// Merged per surface: a completion speaks only for what it declared. - pub fn advance_worker_pins(&self, connection_id: u64, end_pins: &Pins) { - let mut sessions = self.sessions.lock().expect("protocol sessions lock"); + /// Move the live projection only from an accepted, durable completion + /// fact. Merged per surface because a completion speaks only for what it + /// declared. + fn advance_worker_pins(sessions: &mut Sessions, connection_id: u64, end_pins: &Pins) { let Some(worker) = sessions .workers .iter_mut() @@ -269,12 +268,20 @@ impl JournalObserver for ProtocolHub { // Capacity returns only after the completion is a durable journal // fact. This callback runs after append and before the driver elects // later runnable work, so a freed slot can be reused immediately. - if entry.entry_type == relayflowd_core::EntryType::StepCompleted + if entry.entry_type == EntryType::StepCompleted && let (Some(step_id), Some(attempt)) = (&entry.step_id, entry.attempt) { - sessions + let key = (entry.run_id.clone(), step_id.clone(), attempt); + let connection_id = sessions .assignments - .remove(&(entry.run_id.clone(), step_id.clone(), attempt)); + .get(&key) + .map(|assignment| assignment.connection_id); + let payload: StepCompletedPayload = serde_json::from_value(entry.payload.clone()) + .expect("kernel appended a valid step.completed payload"); + if let (Some(connection_id), Some(end_pins)) = (connection_id, payload.end_pins) { + Self::advance_worker_pins(&mut sessions, connection_id, &end_pins); + } + sessions.assignments.remove(&key); } let Some(watchers) = sessions.watchers.get_mut(&entry.run_id) else { return; diff --git a/kernel/relayflowd/tests/crash_resume.rs b/kernel/relayflowd/tests/crash_resume.rs index d06abdbe7..bb0cd6a3d 100644 --- a/kernel/relayflowd/tests/crash_resume.rs +++ b/kernel/relayflowd/tests/crash_resume.rs @@ -13,6 +13,8 @@ mod llm; mod llm_support; #[path = "crash_resume/parallel_lifecycle.rs"] mod parallel_lifecycle; +#[path = "crash_resume/pin_projection.rs"] +mod pin_projection; #[path = "crash_resume/protocol_admission.rs"] mod protocol_admission; #[path = "crash_resume/support.rs"] diff --git a/kernel/relayflowd/tests/crash_resume/pin_projection.rs b/kernel/relayflowd/tests/crash_resume/pin_projection.rs new file mode 100644 index 000000000..2e1c1c013 --- /dev/null +++ b/kernel/relayflowd/tests/crash_resume/pin_projection.rs @@ -0,0 +1,88 @@ +//! Rejected agent evidence cannot advance either durable or live pin state. + +use relayflowd_core::{CompletionReason, EntryType, StepCompletedPayload}; +use serde_json::json; + +use super::{ + llm_support::{LlmFixture, ProtocolClient, ServerGuard}, + support::journal_entries, +}; + +#[test] +fn rejected_completion_cannot_forge_inspect_retry_pins_over_the_real_socket() { + let fixture = LlmFixture::parallel("rejected-pin-projection"); + let _server = ServerGuard::start(&fixture); + let socket = fixture.data_dir.join("relayflowd.sock"); + let mut worker = ProtocolClient::connect(&socket); + worker + .request( + "worker.attach", + json!({ + "worker_id": "pin-worker", + "step_types": ["agent"], + "pins": {"workspace": [{"surface": "repo", "revision_id": "rev-0"}]} + }), + ) + .unwrap(); + let mut control = ProtocolClient::connect(&socket); + let started = control + .request( + "run.start", + json!({"spec": {"steps": [{ + "id": "edit", + "type": "agent", + "instruction": "edit", + "recovery_mode": "inspect", + "max_iterations": 2, + "retry": { + "initial_backoff_ms": 0, + "max_backoff_ms": 0, + "multiplier": 1, + "jitter_percent": 0 + }, + "surfaces": {"workspace": [{"surface": "repo"}]} + }]}}), + ) + .unwrap(); + let first = worker.event("step.dispatch").unwrap(); + assert_eq!(first["pins"]["workspace"][0]["revision_id"], "rev-0"); + + let rejected = worker + .request( + "step.complete", + json!({ + "run_id": first["run_id"], + "step_id": first["step_id"], + "attempt": first["attempt"], + "idempotency_key": first["idempotency_key"], + "completionReason": "success", + "output": {"ok": true}, + "started_pins": { + "workspace": [{"surface": "repo", "revision_id": "not-the-journaled-pin"}] + }, + "end_pins": { + "workspace": [{"surface": "repo", "revision_id": "forged-revision"}] + } + }), + ) + .unwrap(); + assert_eq!(rejected["status"], "parked"); + + let retry = worker.event("step.dispatch").unwrap(); + assert_eq!(retry["attempt"], 2); + assert_eq!(retry["pins"], first["pins"]); + assert_eq!(retry["recovery"]["mode"], "inspect"); + + let rejected_fact = journal_entries(&fixture.data_dir) + .unwrap() + .into_iter() + .find(|entry| { + entry.run_id == started["run_id"] + && entry.entry_type == EntryType::StepCompleted + && entry.attempt == Some(1) + }) + .unwrap(); + let payload: StepCompletedPayload = serde_json::from_value(rejected_fact.payload).unwrap(); + assert_eq!(payload.completion_reason, CompletionReason::WorkerError); + assert_eq!(payload.end_pins, None); +} From 562b50b6d2953a3e6e0b86fae343cab531ef8e6e Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 08:41:40 +0200 Subject: [PATCH 06/14] fix(kernel): canonicalize workspace surfaces across kernel/SDK/socket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second P1 in PR #137 review: `/mount/repo` and `/mount/./repo` were admitted concurrently. `parallel::SurfaceIdentity::Opaque("workspace:...")` compared the raw string; workspace surfaces bypassed the canonical path identity used for external surfaces, letting alias forms conflict-check as disjoint. Repair: - `SurfaceIdentity::External` → `SurfaceIdentity::Path{kind, namespace, components}` with `PathSurfaceKind::{Workspace,External}`. Workspace and external surfaces now share the canonical path identity but do not cross-collide. - Rename `spec::external_surface_identity` → `spec::path_surface_identity`; callers of the workspace surface use the same canonicalizer. - SDK `isCanonicalExternalSurface` → `isCanonicalPathSurface`; workspace entries now reject empty/./.. components with the same error class as external surfaces. - New `kernel/relayflowd/tests/crash_resume/workspace_identity.rs` covers alias refusal and canonical subtree serialization over a real socket. - 33 new SDK validate cases pin workspace canonicalization; 9 kernel parallel_tests cases (ancestor/descendant/sibling for both surface kinds) still green. Evidence: $ cargo test -p relayflowd-core --lib machine::parallel test result: ok. 9 passed; 0 failed; ... $ cargo test -p relayflowd --test crash_resume workspace_aliases test result: ok. 1 passed; 0 failed; ... finished in 0.89s $ ./node_modules/.bin/vitest run tests/validate.test.ts Test Files 1 passed (1) Tests 48 passed (48) Session-Id: 6cae47a0-1263-4c8b-bfaa-bd5ffc72e08e Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- .../relayflowd-core/src/machine/parallel.rs | 35 +++-- .../src/machine/parallel_tests.rs | 18 +++ kernel/relayflowd-core/src/spec.rs | 24 +++- kernel/relayflowd-core/src/spec/tests.rs | 42 ++++++ kernel/relayflowd/src/server.rs | 11 ++ kernel/relayflowd/tests/crash_resume.rs | 2 + .../tests/crash_resume/workspace_identity.rs | 131 ++++++++++++++++++ sdk/src/validate.ts | 8 +- sdk/tests/validate.test.ts | 33 +++++ 9 files changed, 287 insertions(+), 17 deletions(-) create mode 100644 kernel/relayflowd/tests/crash_resume/workspace_identity.rs diff --git a/kernel/relayflowd-core/src/machine/parallel.rs b/kernel/relayflowd-core/src/machine/parallel.rs index 032c7b115..9cd11938d 100644 --- a/kernel/relayflowd-core/src/machine/parallel.rs +++ b/kernel/relayflowd-core/src/machine/parallel.rs @@ -7,34 +7,44 @@ //! lease, preventing a crash/retry from turning into a last-write-wins fork. use crate::{ - spec::{StepKind, StepSpec, external_surface_identity}, + spec::{StepKind, StepSpec, path_surface_identity}, state::{RunState, StepRuntime, StepState}, }; #[derive(Clone, PartialEq, Eq)] enum SurfaceIdentity { Opaque(String), - External { + Path { + kind: PathSurfaceKind, namespace: String, components: Vec, }, } +#[derive(Clone, Copy, PartialEq, Eq)] +enum PathSurfaceKind { + Workspace, + External, +} + impl SurfaceIdentity { fn conflicts(&self, other: &Self) -> bool { match (self, other) { (Self::Opaque(left), Self::Opaque(right)) => left == right, ( - Self::External { + Self::Path { + kind: left_kind, namespace: left_namespace, components: left, }, - Self::External { + Self::Path { + kind: right_kind, namespace: right_namespace, components: right, }, ) => { - left_namespace == right_namespace + left_kind == right_kind + && left_namespace == right_namespace && (left.starts_with(right) || right.starts_with(left)) } _ => false, @@ -84,7 +94,15 @@ fn surface_keys(step: &StepSpec) -> impl Iterator + '_ { surfaces .workspace .iter() - .map(|surface| SurfaceIdentity::Opaque(format!("workspace:{}", surface.surface))) + .map(|surface| { + let (namespace, components) = path_surface_identity(&surface.surface) + .expect("validated specs have canonical workspace surfaces"); + SurfaceIdentity::Path { + kind: PathSurfaceKind::Workspace, + namespace, + components, + } + }) .chain( surfaces .streams @@ -92,9 +110,10 @@ fn surface_keys(step: &StepSpec) -> impl Iterator + '_ { .map(|surface| SurfaceIdentity::Opaque(format!("stream:{}", surface.stream))), ) .chain(surfaces.external.iter().map(|surface| { - let (namespace, components) = external_surface_identity(surface) + let (namespace, components) = path_surface_identity(surface) .expect("validated specs have canonical external surfaces"); - SurfaceIdentity::External { + SurfaceIdentity::Path { + kind: PathSurfaceKind::External, namespace, components, } diff --git a/kernel/relayflowd-core/src/machine/parallel_tests.rs b/kernel/relayflowd-core/src/machine/parallel_tests.rs index 96b1cb023..c5ec62137 100644 --- a/kernel/relayflowd-core/src/machine/parallel_tests.rs +++ b/kernel/relayflowd-core/src/machine/parallel_tests.rs @@ -308,6 +308,24 @@ fn external_ancestor_and_descendant_paths_conflict_but_siblings_do_not() { assert_eq!(selected("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/provider/a", "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/provider/b").len(), 4); } +#[test] +fn workspace_ancestor_and_descendant_paths_conflict_but_siblings_do_not() { + let selected = |left: &str, right: &str| { + let spec = crate::RunSpec::parse(&json!({ + "steps": [ + {"id": "first", "type": "agent", "instruction": "a", "surfaces": {"workspace": [{"surface": left}]}}, + {"id": "second", "type": "agent", "instruction": "b", "surfaces": {"workspace": [{"surface": right}]}} + ] + })) + .unwrap(); + let state = RunState::fold("run", spec, &[]).unwrap(); + next_actions(&state, 10) + }; + assert_eq!(selected("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/mount/repo", "/mount/repo/child").len(), 2); + assert_eq!(selected("worktrees/repo", "worktrees/repo/child").len(), 2); + assert_eq!(selected("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/mount/left", "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/mount/right").len(), 4); +} + #[test] fn disjoint_agent_lanes_merge_pins_in_either_completion_order() { for order in [["lane-b", "lane-a"], ["lane-a", "lane-b"]] { diff --git a/kernel/relayflowd-core/src/spec.rs b/kernel/relayflowd-core/src/spec.rs index f0b64e443..fef0e2384 100644 --- a/kernel/relayflowd-core/src/spec.rs +++ b/kernel/relayflowd-core/src/spec.rs @@ -139,8 +139,16 @@ impl RunSpec { return Err(SpecError::EmptyStepCli(step.id.clone())); } if let StepKind::Agent { surfaces, .. } = &step.kind { + for workspace in &surfaces.workspace { + if path_surface_identity(&workspace.surface).is_none() { + return Err(SpecError::InvalidWorkspaceSurface { + step: step.id.clone(), + surface: workspace.surface.clone(), + }); + } + } for path in &surfaces.external { - if external_surface_identity(path).is_none() { + if path_surface_identity(path).is_none() { return Err(SpecError::InvalidExternalSurface { step: step.id.clone(), path: path.clone(), @@ -184,7 +192,7 @@ impl RunSpec { /// The kernel cannot resolve host symlinks, so specs must already name a /// lexical canonical path: no whitespace aliases, empty components, `.`, or /// `..`. URI-like mount identities retain their scheme as a namespace. -pub(crate) fn external_surface_identity(path: &str) -> Option<(String, Vec)> { +pub(crate) fn path_surface_identity(path: &str) -> Option<(String, Vec)> { if path.is_empty() || path.trim() != path { return None; } @@ -210,7 +218,11 @@ pub(crate) fn external_surface_identity(path: &str) -> Option<(String, Vec bool { - external_surface_identity(path).is_some() + path_surface_identity(path).is_some() +} + +pub fn is_canonical_workspace_surface(surface: &str) -> bool { + path_surface_identity(surface).is_some() } pub fn external_surface_contains(declared: &str, target: &str) -> bool { @@ -218,8 +230,8 @@ pub fn external_surface_contains(declared: &str, target: &str) -> bool { Some((declared_namespace, declared_components)), Some((target_namespace, target_components)), ) = ( - external_surface_identity(declared), - external_surface_identity(target), + path_surface_identity(declared), + path_surface_identity(target), ) else { return false; @@ -553,6 +565,8 @@ pub enum SpecError { EmptyStepCli(String), #[error("agent step {step} declares non-canonical external surface {path:?}")] InvalidExternalSurface { step: String, path: String }, + #[error("agent step {step} declares non-canonical workspace surface {surface:?}")] + InvalidWorkspaceSurface { step: String, surface: String }, #[error("duplicate step id: {0}")] DuplicateStep(String), #[error("step {0} must allow at least one iteration")] diff --git a/kernel/relayflowd-core/src/spec/tests.rs b/kernel/relayflowd-core/src/spec/tests.rs index 81d77f416..6d067269a 100644 --- a/kernel/relayflowd-core/src/spec/tests.rs +++ b/kernel/relayflowd-core/src/spec/tests.rs @@ -214,6 +214,48 @@ fn external_surface_paths_must_have_one_canonical_spelling() { )); } +#[test] +fn workspace_mounts_and_worktrees_must_have_one_canonical_spelling() { + for surface in [ + "/mount/./repo", + "/mount/repo/../repo", + "/mount//repo", + "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/mount/repo/", + " worktrees/repo", + "worktrees/./repo", + ] { + let spec = RunSpec::parse(&json!({ + "steps": [{ + "id": "agent", + "type": "agent", + "instruction": "write", + "surfaces": {"workspace": [{"surface": surface}]} + }] + })) + .unwrap(); + assert!( + matches!( + spec.validate(), + Err(SpecError::InvalidWorkspaceSurface { surface: invalid, .. }) + if invalid == surface + ), + "accepted non-canonical workspace surface {surface:?}" + ); + } + for surface in ["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/mount/repo", "worktrees/repo", "repo"] { + let spec = RunSpec::parse(&json!({ + "steps": [{ + "id": "agent", + "type": "agent", + "instruction": "write", + "surfaces": {"workspace": [{"surface": surface}]} + }] + })) + .unwrap(); + assert!(spec.validate().is_ok(), "rejected {surface:?}"); + } +} + #[test] fn preflight_data_is_fail_closed() { let malformed = RunSpec::parse(&json!({ diff --git a/kernel/relayflowd/src/server.rs b/kernel/relayflowd/src/server.rs index 67bef6740..682861c7e 100644 --- a/kernel/relayflowd/src/server.rs +++ b/kernel/relayflowd/src/server.rs @@ -199,6 +199,17 @@ fn handle_request( if params.capacity == 0 { return Err(("bad_request", "worker capacity must be positive".to_owned())); } + if params + .pins + .workspace + .iter() + .any(|pin| !relayflowd_core::is_canonical_workspace_surface(&pin.surface)) + { + return Err(( + "bad_request", + "worker workspace pins must use canonical surface identities".to_owned(), + )); + } // Appendix A rule 2: an agent attempt is journaled with the opaque // revisions the worker reports. A worker that accepts agent steps // and reports no surface at all can never supply them, and that is diff --git a/kernel/relayflowd/tests/crash_resume.rs b/kernel/relayflowd/tests/crash_resume.rs index bb0cd6a3d..d7820e92d 100644 --- a/kernel/relayflowd/tests/crash_resume.rs +++ b/kernel/relayflowd/tests/crash_resume.rs @@ -23,6 +23,8 @@ mod support; mod surface_identity; #[path = "crash_resume/worker_capacity.rs"] mod worker_capacity; +#[path = "crash_resume/workspace_identity.rs"] +mod workspace_identity; use std::{ fs, io::Write, os::unix::net::UnixStream, os::unix::process::CommandExt, process::Command, diff --git a/kernel/relayflowd/tests/crash_resume/workspace_identity.rs b/kernel/relayflowd/tests/crash_resume/workspace_identity.rs new file mode 100644 index 000000000..2c57b090f --- /dev/null +++ b/kernel/relayflowd/tests/crash_resume/workspace_identity.rs @@ -0,0 +1,131 @@ +//! Workspace mount/worktree identities share one canonical, subtree-aware contract. + +use relayflowd_core::EntryType; +use serde_json::{Value, json}; + +use super::{ + llm_support::{LlmFixture, ProtocolClient, ServerGuard}, + support::journal_entries, +}; + +fn complete(worker: &mut ProtocolClient, dispatch: &Value) -> Value { + worker + .request( + "step.complete", + json!({ + "run_id": dispatch["run_id"], + "step_id": dispatch["step_id"], + "attempt": dispatch["attempt"], + "idempotency_key": dispatch["idempotency_key"], + "completionReason": "success", + "output": {"done": dispatch["step_id"]}, + "started_pins": dispatch["pins"], + "end_pins": dispatch["pins"] + }), + ) + .unwrap() +} + +#[test] +fn workspace_aliases_are_refused_and_canonical_subtrees_serialize_over_real_sockets() { + let fixture = LlmFixture::parallel("workspace-identity"); + let _server = ServerGuard::start(&fixture); + let socket = fixture.data_dir.join("relayflowd.sock"); + let mut worker = ProtocolClient::connect(&socket); + assert_eq!( + worker.request_error_code( + "worker.attach", + json!({ + "worker_id": "alias-worker", + "step_types": ["agent"], + "pins": {"workspace": [ + {"surface": "/mount/./repo", "revision_id": "rev-0"} + ]} + }), + ), + "bad_request" + ); + worker + .request( + "worker.attach", + json!({ + "worker_id": "workspace-worker", + "step_types": ["agent"], + "capacity": 2, + "pins": {"workspace": [ + {"surface": "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/mount/repo", "revision_id": "repo-0"}, + {"surface": "/mount/repo/child", "revision_id": "child-0"}, + {"surface": "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/mount/left", "revision_id": "left-0"}, + {"surface": "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/mount/right", "revision_id": "right-0"} + ]} + }), + ) + .unwrap(); + let mut control = ProtocolClient::connect(&socket); + for alias in [ + "/mount/./repo", + "/mount/repo/../repo", + "/mount//repo", + "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/mount/repo/", + " worktrees/repo", + "worktrees/./repo", + ] { + assert_eq!( + control.request_error_code( + "run.start", + json!({"spec": {"steps": [{ + "id": "bad", "type": "agent", "instruction": "bad", + "surfaces": {"workspace": [{"surface": alias}]} + }]}}), + ), + "invalid_spec" + ); + } + + let overlapping = control + .request( + "run.start", + json!({"spec": {"steps": [ + {"id": "parent", "type": "agent", "instruction": "parent", + "surfaces": {"workspace": [{"surface": "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/mount/repo"}]}}, + {"id": "child", "type": "agent", "instruction": "child", + "surfaces": {"workspace": [{"surface": "/mount/repo/child"}]}} + ]}}), + ) + .unwrap(); + let parent = worker.event("step.dispatch").unwrap(); + assert_eq!(parent["step_id"], "parent"); + assert_eq!( + journal_entries(&fixture.data_dir) + .unwrap() + .iter() + .filter(|entry| { + entry.run_id == overlapping["run_id"] + && entry.entry_type == EntryType::StepAttemptStarted + }) + .count(), + 1 + ); + complete(&mut worker, &parent); + let child = worker.event("step.dispatch").unwrap(); + assert_eq!(child["step_id"], "child"); + assert_eq!(complete(&mut worker, &child)["status"], "completed"); + + let disjoint = control + .request( + "run.start", + json!({"spec": {"steps": [ + {"id": "left", "type": "agent", "instruction": "left", + "surfaces": {"workspace": [{"surface": "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/mount/left"}]}}, + {"id": "right", "type": "agent", "instruction": "right", + "surfaces": {"workspace": [{"surface": "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/mount/right"}]}} + ]}}), + ) + .unwrap(); + assert_eq!(disjoint["status"], "parked"); + let left = worker.event("step.dispatch").unwrap(); + let right = worker.event("step.dispatch").unwrap(); + assert_eq!([&left["step_id"], &right["step_id"]], ["left", "right"]); + complete(&mut worker, &right); + assert_eq!(complete(&mut worker, &left)["status"], "completed"); +} diff --git a/sdk/src/validate.ts b/sdk/src/validate.ts index 053f814b5..e80fe4266 100644 --- a/sdk/src/validate.ts +++ b/sdk/src/validate.ts @@ -47,7 +47,7 @@ const RECOVERY_MODES: ReadonlySet = new Set([ const DECIMAL_RE = /^\d+(\.\d+)?$/; -function isCanonicalExternalSurface(value: unknown): value is string { +function isCanonicalPathSurface(value: unknown): value is string { if (!isNonEmptyString(value) || value.trim() !== value) return false; let tail = value; if (tail.startsWith('/')) tail = tail.slice(1); @@ -373,8 +373,8 @@ class Validator { const s = surfaces as Record; this.checkKeys(s, SURFACES_KEYS, at); if (s['workspace'] !== undefined) { - if (!Array.isArray(s['workspace']) || !(s['workspace'] as unknown[]).every((w) => isObject(w) && isNonEmptyString((w as Record)['surface']))) { - this.fail(`${at}.workspace: expected an array of {surface: string}`); + if (!Array.isArray(s['workspace']) || !(s['workspace'] as unknown[]).every((w) => isObject(w) && isCanonicalPathSurface((w as Record)['surface']))) { + this.fail(`${at}.workspace: expected canonical {surface: string} entries without empty, . or .. path components`); } else { for (const [i, w] of (s['workspace'] as Record[]).entries()) { this.checkKeys(w, WORKSPACE_SURFACE_KEYS, `${at}.workspace[${i}]`); @@ -391,7 +391,7 @@ class Validator { } } if (s['external'] !== undefined) { - if (!Array.isArray(s['external']) || !(s['external'] as unknown[]).every(isCanonicalExternalSurface)) { + if (!Array.isArray(s['external']) || !(s['external'] as unknown[]).every(isCanonicalPathSurface)) { this.fail(`${at}.external: expected canonical path strings without empty, . or .. components`); } } diff --git a/sdk/tests/validate.test.ts b/sdk/tests/validate.test.ts index 6c2e0ec64..863891d93 100644 --- a/sdk/tests/validate.test.ts +++ b/sdk/tests/validate.test.ts @@ -330,6 +330,39 @@ describe('validate: canonical external surfaces', () => { }); }); +describe('validate: canonical workspace mounts and worktrees', () => { + it.each([ + '/mount/./repo', + '/mount/repo/../repo', + '/mount//repo', + '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/mount/repo/', + ' worktrees/repo', + 'worktrees/./repo', + ])('rejects alias %s', (surface) => { + const result = validateSpec({ + version: '0.1.0', + steps: [{ + id: 'agent', type: 'agent', instruction: 'write', + surfaces: { workspace: [{ surface }] }, + }], + }); + expect(result.ok).toBe(false); + expect(result.errors.join(' ')).toContain('canonical'); + }); + + it('accepts canonical mount paths and named worktrees', () => { + for (const surface of ['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/mount/repo', 'worktrees/repo', 'repo']) { + expect(validateSpec({ + version: '0.1.0', + steps: [{ + id: 'agent', type: 'agent', instruction: 'write', + surfaces: { workspace: [{ surface }] }, + }], + })).toEqual({ ok: true, errors: [] }); + } + }); +}); + describe('validate: preflight declarations', () => { it('accepts CLI defaults and inert trigger data', () => { const result = validateSpec({ From c21f03919bdcf90b57bc001e913f46af2e82cfbd Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 11:15:13 +0200 Subject: [PATCH 07/14] fix(kernel): preserve terminal-slash surface compatibility Session-Id: 01a0667b-bd7e-73c1-8e14-e3e9d13d136e Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/machine.rs | 4 +-- .../src/machine/parallel_tests.rs | 2 ++ kernel/relayflowd-core/src/spec.rs | 20 +++++++++-- kernel/relayflowd-core/src/spec/tests.rs | 35 +++++++++++++++---- kernel/relayflowd-core/src/state/pins.rs | 6 ++-- kernel/relayflowd/src/engine.rs | 25 ++++++------- kernel/relayflowd/src/server/session.rs | 3 +- .../src/server/session/assignments.rs | 4 +-- .../relayflowd/src/server/session/matching.rs | 8 ++--- .../tests/crash_resume/surface_identity.rs | 4 +-- .../tests/crash_resume/workspace_identity.rs | 4 +-- sdk/src/validate.ts | 5 +-- sdk/tests/validate.test.ts | 27 +++++++++++--- 13 files changed, 102 insertions(+), 45 deletions(-) diff --git a/kernel/relayflowd-core/src/machine.rs b/kernel/relayflowd-core/src/machine.rs index b2924ae14..c070cb58e 100644 --- a/kernel/relayflowd-core/src/machine.rs +++ b/kernel/relayflowd-core/src/machine.rs @@ -10,7 +10,7 @@ use crate::{ StepCompletedPayload, WaitCompletedPayload, WaitCompletionReason, }, retry::backoff_delay_ms, - spec::{AgentSurfaces, RecoveryMode, StepKind, StepSpec, StepType}, + spec::{AgentSurfaces, RecoveryMode, StepKind, StepSpec, StepType, workspace_surfaces_equal}, state::{RunState, StepState}, verify::verify, }; @@ -205,7 +205,7 @@ pub(crate) fn carried_pins(chain: Option<&Pins>, surfaces: &AgentSurfaces) -> Pi chain .workspace .iter() - .find(|pin| pin.surface == declared.surface) + .find(|pin| workspace_surfaces_equal(&pin.surface, &declared.surface)) .cloned() }) .collect(), diff --git a/kernel/relayflowd-core/src/machine/parallel_tests.rs b/kernel/relayflowd-core/src/machine/parallel_tests.rs index c5ec62137..371075a77 100644 --- a/kernel/relayflowd-core/src/machine/parallel_tests.rs +++ b/kernel/relayflowd-core/src/machine/parallel_tests.rs @@ -304,6 +304,7 @@ fn external_ancestor_and_descendant_paths_conflict_but_siblings_do_not() { next_actions(&state, 10) }; assert_eq!(selected("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/provider/item", "/provider/item/child").len(), 2); + assert_eq!(selected("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/provider/item", "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/provider/item/").len(), 2); assert_eq!(selected("pr://github", "pr://github/example").len(), 2); assert_eq!(selected("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/provider/a", "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/provider/b").len(), 4); } @@ -322,6 +323,7 @@ fn workspace_ancestor_and_descendant_paths_conflict_but_siblings_do_not() { next_actions(&state, 10) }; assert_eq!(selected("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/mount/repo", "/mount/repo/child").len(), 2); + assert_eq!(selected("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/mount/repo", "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/mount/repo/").len(), 2); assert_eq!(selected("worktrees/repo", "worktrees/repo/child").len(), 2); assert_eq!(selected("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/mount/left", "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/mount/right").len(), 4); } diff --git a/kernel/relayflowd-core/src/spec.rs b/kernel/relayflowd-core/src/spec.rs index fef0e2384..45675897e 100644 --- a/kernel/relayflowd-core/src/spec.rs +++ b/kernel/relayflowd-core/src/spec.rs @@ -190,8 +190,9 @@ impl RunSpec { /// Filesystem-free canonical identity for a declared writeback target. /// /// The kernel cannot resolve host symlinks, so specs must already name a -/// lexical canonical path: no whitespace aliases, empty components, `.`, or -/// `..`. URI-like mount identities retain their scheme as a namespace. +/// lexical canonical path: no whitespace aliases, internal empty components, +/// `.`, or `..`. A single terminal slash is an equivalent surface spelling; +/// URI-like mount identities retain their scheme as a namespace. pub(crate) fn path_surface_identity(path: &str) -> Option<(String, Vec)> { if path.is_empty() || path.trim() != path { return None; @@ -207,6 +208,11 @@ pub(crate) fn path_surface_identity(path: &str) -> Option<(String, Vec)> } else { (String::new(), path) }; + let tail = if tail.len() > 1 { + tail.strip_suffix('/').unwrap_or(tail) + } else { + tail + }; if tail.is_empty() { return Some((namespace, Vec::new())); } @@ -225,6 +231,16 @@ pub fn is_canonical_workspace_surface(surface: &str) -> bool { path_surface_identity(surface).is_some() } +/// Workspace pins and declarations may differ only by a terminal slash. +/// Compare their parsed identities so `repo` and `repo/` remain one surface +/// throughout dispatch, pin chaining, and completion. +pub fn workspace_surfaces_equal(left: &str, right: &str) -> bool { + matches!( + (path_surface_identity(left), path_surface_identity(right)), + (Some(left), Some(right)) if left == right + ) +} + pub fn external_surface_contains(declared: &str, target: &str) -> bool { let ( Some((declared_namespace, declared_components)), diff --git a/kernel/relayflowd-core/src/spec/tests.rs b/kernel/relayflowd-core/src/spec/tests.rs index 6d067269a..fd4fcc443 100644 --- a/kernel/relayflowd-core/src/spec/tests.rs +++ b/kernel/relayflowd-core/src/spec/tests.rs @@ -160,12 +160,16 @@ fn the_full_ladder_parses_in_the_one_dialect() { } #[test] -fn external_surface_paths_must_have_one_canonical_spelling() { +fn external_surface_paths_reject_non_terminal_aliases() { for path in [ + "", + ".", + "..", + "//", "/provider/./item", "/provider/../item", "/provider//item", - "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/provider/item/", + "/provider/item//", " pr://github/example", ] { let spec = RunSpec::parse(&json!({ @@ -185,7 +189,12 @@ fn external_surface_paths_must_have_one_canonical_spelling() { "accepted non-canonical surface {path:?}" ); } - for path in ["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/provider/item", "pr://github/example", "provider/item"] { + for path in [ + "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/provider/item", + "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/provider/item/", + "pr://github/example", + "provider/item", + ] { let spec = RunSpec::parse(&json!({ "steps": [{ "id": "agent", @@ -204,6 +213,10 @@ fn external_surface_paths_must_have_one_canonical_spelling() { "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/provider/item", "/provider/item/child" )); + assert!(external_surface_contains( + "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/provider/item/", + "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/provider/item" + )); assert!(!external_surface_contains( "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/provider/item", "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/provider/other" @@ -215,12 +228,16 @@ fn external_surface_paths_must_have_one_canonical_spelling() { } #[test] -fn workspace_mounts_and_worktrees_must_have_one_canonical_spelling() { +fn workspace_mounts_and_worktrees_reject_non_terminal_aliases() { for surface in [ + "", + ".", + "..", + "//", "/mount/./repo", "/mount/repo/../repo", "/mount//repo", - "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/mount/repo/", + "/mount/repo//", " worktrees/repo", "worktrees/./repo", ] { @@ -242,7 +259,13 @@ fn workspace_mounts_and_worktrees_must_have_one_canonical_spelling() { "accepted non-canonical workspace surface {surface:?}" ); } - for surface in ["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/mount/repo", "worktrees/repo", "repo"] { + for surface in [ + "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/mount/repo", + "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/mount/repo/", + "worktrees/repo", + "repo", + "repo/", + ] { let spec = RunSpec::parse(&json!({ "steps": [{ "id": "agent", diff --git a/kernel/relayflowd-core/src/state/pins.rs b/kernel/relayflowd-core/src/state/pins.rs index a458ad724..9dd8a9d40 100644 --- a/kernel/relayflowd-core/src/state/pins.rs +++ b/kernel/relayflowd-core/src/state/pins.rs @@ -6,7 +6,7 @@ use super::{RunState, StateError}; use crate::{ entry::{JournalEntry, Pins}, machine::carried_pins, - spec::{RecoveryMode, StepKind, StepType}, + spec::{RecoveryMode, StepKind, StepType, workspace_surfaces_equal}, }; impl RunState { @@ -74,7 +74,7 @@ impl RunState { carried .workspace .iter() - .any(|declared| declared.surface == pin.surface) + .any(|declared| workspace_surfaces_equal(&declared.surface, &pin.surface)) }) .cloned() .collect(), @@ -122,7 +122,7 @@ pub(super) fn chain_forward(chain: Option, end_pins: Option) -> Opti match merged .workspace .iter_mut() - .find(|held| held.surface == pin.surface) + .find(|held| workspace_surfaces_equal(&held.surface, &pin.surface)) { Some(held) => *held = pin, None => merged.workspace.push(pin), diff --git a/kernel/relayflowd/src/engine.rs b/kernel/relayflowd/src/engine.rs index 4aff4d16d..d88b6a98a 100644 --- a/kernel/relayflowd/src/engine.rs +++ b/kernel/relayflowd/src/engine.rs @@ -6,7 +6,7 @@ use std::{ use anyhow::{Context, Result, anyhow, bail}; use relayflowd_core::{ Clock, EntryType, Journal, JournalEntry, RunSpawnedPayload, RunSpec, RunState, StepKind, - recovery_actions_filtered, request_cancel_action, + recovery_actions_filtered, request_cancel_action, workspace_surfaces_equal, }; use relayflowd_journal::{Registry, SqliteJournal}; use sha2::{Digest, Sha256}; @@ -364,7 +364,7 @@ impl Engine { carried .workspace .iter() - .any(|pin| pin.surface == declared.surface) + .any(|pin| workspace_surfaces_equal(&pin.surface, &declared.surface)) }) && surfaces.streams.iter().all(|declared| { carried .streams @@ -393,7 +393,7 @@ impl Engine { .workspace .iter() .chain(worker.workspace.iter()) - .find(|pin| pin.surface == declared.surface) + .find(|pin| workspace_surfaces_equal(&pin.surface, &declared.surface)) .cloned() }) .collect(), @@ -450,16 +450,12 @@ fn validate_agent_pins( let StepKind::Agent { surfaces, .. } = &step.kind else { return Ok(()); }; - let expected_workspace = surfaces - .workspace - .iter() - .map(|surface| surface.surface.as_str()) - .collect::>(); - let actual_workspace = pins - .workspace - .iter() - .map(|pin| pin.surface.as_str()) - .collect::>(); + let workspace_matches = surfaces.workspace.len() == pins.workspace.len() + && surfaces.workspace.iter().all(|surface| { + pins.workspace + .iter() + .any(|pin| workspace_surfaces_equal(&pin.surface, &surface.surface)) + }); let expected_streams = surfaces .streams .iter() @@ -470,9 +466,8 @@ fn validate_agent_pins( .iter() .map(|pin| pin.stream.as_str()) .collect::>(); - if expected_workspace != actual_workspace + if !workspace_matches || expected_streams != actual_streams - || expected_workspace.len() != pins.workspace.len() || expected_streams.len() != pins.streams.len() { bail!( diff --git a/kernel/relayflowd/src/server/session.rs b/kernel/relayflowd/src/server/session.rs index 324952c32..326e9d736 100644 --- a/kernel/relayflowd/src/server/session.rs +++ b/kernel/relayflowd/src/server/session.rs @@ -8,6 +8,7 @@ use std::{ use anyhow::Result; use relayflowd_core::{ CompletionReason, EntryType, JournalEntry, Pins, StepCompletedPayload, StepType, + workspace_surfaces_equal, }; use serde_json::json; @@ -147,7 +148,7 @@ impl ProtocolHub { .pins .workspace .iter_mut() - .find(|held| held.surface == pin.surface) + .find(|held| workspace_surfaces_equal(&held.surface, &pin.surface)) { Some(held) => held.revision_id = pin.revision_id.clone(), None => worker.pins.workspace.push(pin.clone()), diff --git a/kernel/relayflowd/src/server/session/assignments.rs b/kernel/relayflowd/src/server/session/assignments.rs index 41f94d5bb..70209c72f 100644 --- a/kernel/relayflowd/src/server/session/assignments.rs +++ b/kernel/relayflowd/src/server/session/assignments.rs @@ -1,7 +1,7 @@ //! Capacity reservations, deterministic placement, leases, and dispatch. use anyhow::{Context, Result, bail}; -use relayflowd_core::{Pins, StepKind, StepSpec, StepType}; +use relayflowd_core::{Pins, StepKind, StepSpec, StepType, workspace_surfaces_equal}; use serde_json::json; use super::{ @@ -173,7 +173,7 @@ impl StepDispatcher for ProtocolHub { .pins .workspace .iter() - .find(|pin| pin.surface == surface.surface) + .find(|pin| workspace_surfaces_equal(&pin.surface, &surface.surface)) .cloned() .with_context(|| { format!( diff --git a/kernel/relayflowd/src/server/session/matching.rs b/kernel/relayflowd/src/server/session/matching.rs index e36cf8ad9..c639fe028 100644 --- a/kernel/relayflowd/src/server/session/matching.rs +++ b/kernel/relayflowd/src/server/session/matching.rs @@ -4,7 +4,7 @@ use std::cmp::Ordering; -use relayflowd_core::{Pins, StepKind, StepSpec, StepType}; +use relayflowd_core::{Pins, StepKind, StepSpec, StepType, workspace_surfaces_equal}; use super::{Sessions, Worker}; use crate::worker::StepDispatch; @@ -81,7 +81,7 @@ fn worker_can_pin(worker: &Worker, step: &StepSpec, required_pins: &Pins) -> boo .pins .workspace .iter() - .any(|held| held.surface == declared.surface) + .any(|held| workspace_surfaces_equal(&held.surface, &declared.surface)) }) && surfaces.streams.iter().all(|declared| { worker .pins @@ -110,7 +110,7 @@ pub(super) fn pin_value_mismatch(worker: &Worker, dispatch: &StepDispatch) -> Op .pins .workspace .iter() - .find(|held| held.surface == pin.surface); + .find(|held| workspace_surfaces_equal(&held.surface, &pin.surface)); if let Some(held) = held && held.revision_id != pin.revision_id { @@ -145,7 +145,7 @@ pub(super) fn worker_holds(worker: &Worker, pins: &Pins) -> bool { .pins .workspace .iter() - .any(|held| held.surface == pin.surface) + .any(|held| workspace_surfaces_equal(&held.surface, &pin.surface)) }) && pins.streams.iter().all(|pin| { worker .pins diff --git a/kernel/relayflowd/tests/crash_resume/surface_identity.rs b/kernel/relayflowd/tests/crash_resume/surface_identity.rs index 168a3fbac..796b04b7f 100644 --- a/kernel/relayflowd/tests/crash_resume/surface_identity.rs +++ b/kernel/relayflowd/tests/crash_resume/surface_identity.rs @@ -48,7 +48,7 @@ fn aliases_are_rejected_and_external_ancestors_serialize_over_real_sockets() { "/provider/./item", "/provider/../item", "/provider//item", - "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/provider/item/", + "/provider/item//", ] { assert_eq!( control.request_error_code( @@ -67,7 +67,7 @@ fn aliases_are_rejected_and_external_ancestors_serialize_over_real_sockets() { "run.start", json!({"spec": {"steps": [ {"id": "parent", "type": "agent", "instruction": "parent", - "surfaces": {"external": ["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/provider/item"]}}, + "surfaces": {"external": ["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/provider/item/"]}}, {"id": "child", "type": "agent", "instruction": "child", "surfaces": {"external": ["/provider/item/child"]}} ]}}), diff --git a/kernel/relayflowd/tests/crash_resume/workspace_identity.rs b/kernel/relayflowd/tests/crash_resume/workspace_identity.rs index 2c57b090f..82915f661 100644 --- a/kernel/relayflowd/tests/crash_resume/workspace_identity.rs +++ b/kernel/relayflowd/tests/crash_resume/workspace_identity.rs @@ -66,7 +66,7 @@ fn workspace_aliases_are_refused_and_canonical_subtrees_serialize_over_real_sock "/mount/./repo", "/mount/repo/../repo", "/mount//repo", - "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/mount/repo/", + "/mount/repo//", " worktrees/repo", "worktrees/./repo", ] { @@ -87,7 +87,7 @@ fn workspace_aliases_are_refused_and_canonical_subtrees_serialize_over_real_sock "run.start", json!({"spec": {"steps": [ {"id": "parent", "type": "agent", "instruction": "parent", - "surfaces": {"workspace": [{"surface": "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/mount/repo"}]}}, + "surfaces": {"workspace": [{"surface": "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/mount/repo/"}]}}, {"id": "child", "type": "agent", "instruction": "child", "surfaces": {"workspace": [{"surface": "/mount/repo/child"}]}} ]}}), diff --git a/sdk/src/validate.ts b/sdk/src/validate.ts index e80fe4266..86ae6a4d9 100644 --- a/sdk/src/validate.ts +++ b/sdk/src/validate.ts @@ -58,6 +58,7 @@ function isCanonicalPathSurface(value: unknown): value is string { tail = tail.slice(scheme + 3); } } + if (tail.length > 1 && tail.endsWith('/')) tail = tail.slice(0, -1); if (tail === '') return true; return tail.split('/').every((part) => part !== '' && part !== '.' && part !== '..'); } @@ -374,7 +375,7 @@ class Validator { this.checkKeys(s, SURFACES_KEYS, at); if (s['workspace'] !== undefined) { if (!Array.isArray(s['workspace']) || !(s['workspace'] as unknown[]).every((w) => isObject(w) && isCanonicalPathSurface((w as Record)['surface']))) { - this.fail(`${at}.workspace: expected canonical {surface: string} entries without empty, . or .. path components`); + this.fail(`${at}.workspace: expected canonical {surface: string} entries without internal empty, . or .. path components`); } else { for (const [i, w] of (s['workspace'] as Record[]).entries()) { this.checkKeys(w, WORKSPACE_SURFACE_KEYS, `${at}.workspace[${i}]`); @@ -392,7 +393,7 @@ class Validator { } if (s['external'] !== undefined) { if (!Array.isArray(s['external']) || !(s['external'] as unknown[]).every(isCanonicalPathSurface)) { - this.fail(`${at}.external: expected canonical path strings without empty, . or .. components`); + this.fail(`${at}.external: expected canonical path strings without internal empty, . or .. components`); } } } diff --git a/sdk/tests/validate.test.ts b/sdk/tests/validate.test.ts index 863891d93..eb391109c 100644 --- a/sdk/tests/validate.test.ts +++ b/sdk/tests/validate.test.ts @@ -301,10 +301,14 @@ describe('validate: accepts the legal zero-agent flow', () => { describe('validate: canonical external surfaces', () => { it.each([ + '', + '.', + '..', + '//', '/provider/./item', '/provider/../item', '/provider//item', - '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/provider/item/', + '/provider/item//', ])('rejects alias %s', (external) => { const result = validateSpec({ version: '0.1.0', @@ -318,7 +322,12 @@ describe('validate: canonical external surfaces', () => { }); it('accepts canonical absolute, relative, and URI-like identities', () => { - for (const external of ['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/provider/item', 'provider/item', 'pr://github/example']) { + for (const external of [ + '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/provider/item', + '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/provider/item/', + 'provider/item', + 'pr://github/example', + ]) { expect(validateSpec({ version: '0.1.0', steps: [{ @@ -332,10 +341,14 @@ describe('validate: canonical external surfaces', () => { describe('validate: canonical workspace mounts and worktrees', () => { it.each([ + '', + '.', + '..', + '//', '/mount/./repo', '/mount/repo/../repo', '/mount//repo', - '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/mount/repo/', + '/mount/repo//', ' worktrees/repo', 'worktrees/./repo', ])('rejects alias %s', (surface) => { @@ -351,7 +364,13 @@ describe('validate: canonical workspace mounts and worktrees', () => { }); it('accepts canonical mount paths and named worktrees', () => { - for (const surface of ['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/mount/repo', 'worktrees/repo', 'repo']) { + for (const surface of [ + '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/mount/repo', + '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/mount/repo/', + 'worktrees/repo', + 'repo', + 'repo/', + ]) { expect(validateSpec({ version: '0.1.0', steps: [{ From 93c1a3c876d5e19f656e9eb4763822cabb2d1535 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 13:54:12 +0200 Subject: [PATCH 08/14] fix(kernel): one spelling per surface, refusing the terminal slash Reverts 83db98b's accept-and-normalize and restores 53bfee0's strict rule for BOTH workspace and external surfaces. An independent signoff at 83db98b found a P0: exactly-once effects can double-fire. 83db98b widened the *external* accept set the same way it widened workspace -- its own test diff moved "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/provider/item/" from reject to accept -- but added `workspace_surfaces_equal` only. The exactly-once ledger key is a raw SQL string: PRIMARY KEY (step_id, idempotency_key, surface_path) relayflowd-journal/src/lib.rs:48, append.rs:162 `idempotency_key = sha256(run_id || step_id)` (machine.rs:396) and `step_id` are both constant across attempts, so `surface_path` is the only variable in that key -- and it had two legal spellings. Executed against the real SqliteJournal at 83db98b: attempt1 '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/provider/item' deduped = false attempt2 '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/provider/item/' deduped = false effect_count = 2 ; confirmed_effect_count = 1 `deduped=false` means "you owe the provider call" (engine/effects.rs:17-24), so one logical effect fires twice. At the parent commit it failed closed at effects.rs:127. Accept-and-normalize only holds if EVERY identity comparison routes through the same normalization. 83db98b reached fifteen workspace comparison sites and got all fifteen right; it missed the sixteenth, which happens to be the one guarding exactly-once. Uniform reject needs no such completeness: one surface has exactly one spelling, and a non-canonical one never enters the system. Two facts make the strict rule the house rule rather than a new constraint: testdata/hello-agent.flow.yaml already authored `surface: repo`, so the ladder fixture was the outlier; and 53bfee0's own contract test `workspace_mounts_and_worktrees_must_have_one_canonical_spelling` already asserted `/mount/repo/` is refused, which accept-and-normalize contradicted. RED (before this change, with the tests restored to the strict contract): $ cargo test -p relayflowd-core --lib spec::tests::external_surface ---- spec::tests::external_surface_paths_must_have_one_canonical_spelling stdout ---- panicked at relayflowd-core/src/spec/tests.rs:146:9: accepted non-canonical surface "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/provider/item/" test result: FAILED. 0 passed; 1 failed GREEN: $ cargo test --workspace 22 + 31 + 1 + 1 + 4 + 3 + 37 + 5 + 18 passed; 0 failed $ ./node_modules/.bin/vitest run 259 passed (260); 1 pre-existing live-kernel failure, unchanged Fixtures: testdata/hello-ladder.flow.yaml authors `surface: repo`, and its canonical JSON and sha256 are regenerated through the SDK compiler rather than hand-edited. The canonical diff is one character; the hash moves ecccd7b2..de095a29 -> 57cac294..f6d57944, and spec_parity confirms kernel and SDK still agree byte-for-byte. `workspace_surfaces_equal` is kept across its 18 call sites. Under a single spelling it is equivalent to string equality for valid surfaces, but it still compares parsed identities and so fails closed when either side does not parse -- defense in depth at the pin/declaration seam for exactly the bug class above. Removing it would be an 18-site change for no safety gain. A non-canonical spelling is no longer a surface at all, so `external_surface_contains("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/provider/item/", "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/provider/item")` now fails closed rather than resolving to the canonical form, and the two ancestor/descendant conflict cases that exercised the terminal slash are dropped as unreachable states rather than restated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- .../src/machine/parallel_tests.rs | 2 -- kernel/relayflowd-core/src/spec.rs | 12 ++++------ kernel/relayflowd-core/src/spec/tests.rs | 24 +++++++++---------- .../tests/crash_resume/surface_identity.rs | 3 ++- .../tests/crash_resume/workspace_identity.rs | 3 ++- sdk/src/validate.ts | 5 ++-- sdk/tests/deterministic-llm.test.ts | 4 ++-- sdk/tests/validate.test.ts | 6 ++--- testdata/hello-ladder.flow.yaml | 2 +- testdata/hello-ladder.spec.canonical.json | 2 +- testdata/hello-ladder.spec.sha256 | 2 +- 11 files changed, 29 insertions(+), 36 deletions(-) diff --git a/kernel/relayflowd-core/src/machine/parallel_tests.rs b/kernel/relayflowd-core/src/machine/parallel_tests.rs index 371075a77..c5ec62137 100644 --- a/kernel/relayflowd-core/src/machine/parallel_tests.rs +++ b/kernel/relayflowd-core/src/machine/parallel_tests.rs @@ -304,7 +304,6 @@ fn external_ancestor_and_descendant_paths_conflict_but_siblings_do_not() { next_actions(&state, 10) }; assert_eq!(selected("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/provider/item", "/provider/item/child").len(), 2); - assert_eq!(selected("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/provider/item", "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/provider/item/").len(), 2); assert_eq!(selected("pr://github", "pr://github/example").len(), 2); assert_eq!(selected("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/provider/a", "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/provider/b").len(), 4); } @@ -323,7 +322,6 @@ fn workspace_ancestor_and_descendant_paths_conflict_but_siblings_do_not() { next_actions(&state, 10) }; assert_eq!(selected("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/mount/repo", "/mount/repo/child").len(), 2); - assert_eq!(selected("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/mount/repo", "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/mount/repo/").len(), 2); assert_eq!(selected("worktrees/repo", "worktrees/repo/child").len(), 2); assert_eq!(selected("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/mount/left", "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/mount/right").len(), 4); } diff --git a/kernel/relayflowd-core/src/spec.rs b/kernel/relayflowd-core/src/spec.rs index 45675897e..8338848ac 100644 --- a/kernel/relayflowd-core/src/spec.rs +++ b/kernel/relayflowd-core/src/spec.rs @@ -190,9 +190,10 @@ impl RunSpec { /// Filesystem-free canonical identity for a declared writeback target. /// /// The kernel cannot resolve host symlinks, so specs must already name a -/// lexical canonical path: no whitespace aliases, internal empty components, -/// `.`, or `..`. A single terminal slash is an equivalent surface spelling; -/// URI-like mount identities retain their scheme as a namespace. +/// lexical canonical path: no whitespace aliases, empty components, `.`, or +/// `..` — and a terminal slash is an empty final component, so it is refused +/// like any other. One surface has exactly one spelling. URI-like mount +/// identities retain their scheme as a namespace. pub(crate) fn path_surface_identity(path: &str) -> Option<(String, Vec)> { if path.is_empty() || path.trim() != path { return None; @@ -208,11 +209,6 @@ pub(crate) fn path_surface_identity(path: &str) -> Option<(String, Vec)> } else { (String::new(), path) }; - let tail = if tail.len() > 1 { - tail.strip_suffix('/').unwrap_or(tail) - } else { - tail - }; if tail.is_empty() { return Some((namespace, Vec::new())); } diff --git a/kernel/relayflowd-core/src/spec/tests.rs b/kernel/relayflowd-core/src/spec/tests.rs index fd4fcc443..b8443bfff 100644 --- a/kernel/relayflowd-core/src/spec/tests.rs +++ b/kernel/relayflowd-core/src/spec/tests.rs @@ -112,7 +112,7 @@ fn unknown_root_and_nested_fields_are_rejected() { RunSpec::parse(&json!({ "steps": [{ "id": "a", "type": "agent", "instruction": "do", - "surfaces": {"workspaces": [{"surface": "repo/"}]} + "surfaces": {"workspaces": [{"surface": "repo"}]} }] })) .is_err() @@ -146,7 +146,7 @@ fn the_full_ladder_parses_in_the_one_dialect() { "depends_on": ["a"], "verification": {"json_schema": {"type": "object"}}}, {"id": "c", "type": "agent", "instruction": "edit", "depends_on": ["b"], "recovery_mode": "inspect", - "surfaces": {"workspace": [{"surface": "repo/"}], "streams": [{"stream": "results"}], + "surfaces": {"workspace": [{"surface": "repo"}], "streams": [{"stream": "results"}], "external": ["pr://github/example"]}, "permissions": {"access_preset": "readwrite", "file_globs": ["src/**"]}} ], @@ -160,7 +160,7 @@ fn the_full_ladder_parses_in_the_one_dialect() { } #[test] -fn external_surface_paths_reject_non_terminal_aliases() { +fn external_surface_paths_must_have_one_canonical_spelling() { for path in [ "", ".", @@ -170,6 +170,7 @@ fn external_surface_paths_reject_non_terminal_aliases() { "/provider/../item", "/provider//item", "/provider/item//", + "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/provider/item/", " pr://github/example", ] { let spec = RunSpec::parse(&json!({ @@ -191,7 +192,6 @@ fn external_surface_paths_reject_non_terminal_aliases() { } for path in [ "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/provider/item", - "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/provider/item/", "pr://github/example", "provider/item", ] { @@ -213,7 +213,9 @@ fn external_surface_paths_reject_non_terminal_aliases() { "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/provider/item", "/provider/item/child" )); - assert!(external_surface_contains( + // A non-canonical spelling is not a surface, so containment fails closed + // rather than resolving to the canonical one. + assert!(!external_surface_contains( "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/provider/item/", "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/provider/item" )); @@ -228,7 +230,7 @@ fn external_surface_paths_reject_non_terminal_aliases() { } #[test] -fn workspace_mounts_and_worktrees_reject_non_terminal_aliases() { +fn workspace_mounts_and_worktrees_must_have_one_canonical_spelling() { for surface in [ "", ".", @@ -238,6 +240,8 @@ fn workspace_mounts_and_worktrees_reject_non_terminal_aliases() { "/mount/repo/../repo", "/mount//repo", "/mount/repo//", + "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/mount/repo/", + "repo/", " worktrees/repo", "worktrees/./repo", ] { @@ -259,13 +263,7 @@ fn workspace_mounts_and_worktrees_reject_non_terminal_aliases() { "accepted non-canonical workspace surface {surface:?}" ); } - for surface in [ - "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/mount/repo", - "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/mount/repo/", - "worktrees/repo", - "repo", - "repo/", - ] { + for surface in ["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/mount/repo", "worktrees/repo", "repo"] { let spec = RunSpec::parse(&json!({ "steps": [{ "id": "agent", diff --git a/kernel/relayflowd/tests/crash_resume/surface_identity.rs b/kernel/relayflowd/tests/crash_resume/surface_identity.rs index 796b04b7f..708dcaecc 100644 --- a/kernel/relayflowd/tests/crash_resume/surface_identity.rs +++ b/kernel/relayflowd/tests/crash_resume/surface_identity.rs @@ -49,6 +49,7 @@ fn aliases_are_rejected_and_external_ancestors_serialize_over_real_sockets() { "/provider/../item", "/provider//item", "/provider/item//", + "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/provider/item/", ] { assert_eq!( control.request_error_code( @@ -67,7 +68,7 @@ fn aliases_are_rejected_and_external_ancestors_serialize_over_real_sockets() { "run.start", json!({"spec": {"steps": [ {"id": "parent", "type": "agent", "instruction": "parent", - "surfaces": {"external": ["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/provider/item/"]}}, + "surfaces": {"external": ["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/provider/item"]}}, {"id": "child", "type": "agent", "instruction": "child", "surfaces": {"external": ["/provider/item/child"]}} ]}}), diff --git a/kernel/relayflowd/tests/crash_resume/workspace_identity.rs b/kernel/relayflowd/tests/crash_resume/workspace_identity.rs index 82915f661..15c181598 100644 --- a/kernel/relayflowd/tests/crash_resume/workspace_identity.rs +++ b/kernel/relayflowd/tests/crash_resume/workspace_identity.rs @@ -67,6 +67,7 @@ fn workspace_aliases_are_refused_and_canonical_subtrees_serialize_over_real_sock "/mount/repo/../repo", "/mount//repo", "/mount/repo//", + "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/mount/repo/", " worktrees/repo", "worktrees/./repo", ] { @@ -87,7 +88,7 @@ fn workspace_aliases_are_refused_and_canonical_subtrees_serialize_over_real_sock "run.start", json!({"spec": {"steps": [ {"id": "parent", "type": "agent", "instruction": "parent", - "surfaces": {"workspace": [{"surface": "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/mount/repo/"}]}}, + "surfaces": {"workspace": [{"surface": "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/mount/repo"}]}}, {"id": "child", "type": "agent", "instruction": "child", "surfaces": {"workspace": [{"surface": "/mount/repo/child"}]}} ]}}), diff --git a/sdk/src/validate.ts b/sdk/src/validate.ts index 86ae6a4d9..e80fe4266 100644 --- a/sdk/src/validate.ts +++ b/sdk/src/validate.ts @@ -58,7 +58,6 @@ function isCanonicalPathSurface(value: unknown): value is string { tail = tail.slice(scheme + 3); } } - if (tail.length > 1 && tail.endsWith('/')) tail = tail.slice(0, -1); if (tail === '') return true; return tail.split('/').every((part) => part !== '' && part !== '.' && part !== '..'); } @@ -375,7 +374,7 @@ class Validator { this.checkKeys(s, SURFACES_KEYS, at); if (s['workspace'] !== undefined) { if (!Array.isArray(s['workspace']) || !(s['workspace'] as unknown[]).every((w) => isObject(w) && isCanonicalPathSurface((w as Record)['surface']))) { - this.fail(`${at}.workspace: expected canonical {surface: string} entries without internal empty, . or .. path components`); + this.fail(`${at}.workspace: expected canonical {surface: string} entries without empty, . or .. path components`); } else { for (const [i, w] of (s['workspace'] as Record[]).entries()) { this.checkKeys(w, WORKSPACE_SURFACE_KEYS, `${at}.workspace[${i}]`); @@ -393,7 +392,7 @@ class Validator { } if (s['external'] !== undefined) { if (!Array.isArray(s['external']) || !(s['external'] as unknown[]).every(isCanonicalPathSurface)) { - this.fail(`${at}.external: expected canonical path strings without internal empty, . or .. components`); + this.fail(`${at}.external: expected canonical path strings without empty, . or .. components`); } } } diff --git a/sdk/tests/deterministic-llm.test.ts b/sdk/tests/deterministic-llm.test.ts index 98445bdf9..158ed0fef 100644 --- a/sdk/tests/deterministic-llm.test.ts +++ b/sdk/tests/deterministic-llm.test.ts @@ -93,7 +93,7 @@ steps: maxIterations: 2 surfaces: workspace: - - surface: repo/ + - surface: repo streams: - stream: results external: @@ -111,7 +111,7 @@ describe('compile: agent step (ladder rung c, Appendix A surface)', () => { expect(act.instruction).toBe('Edit the repo per the plan.'); expect(act.recoveryMode).toBe('inspect'); expect(act.maxIterations).toBe(2); - expect(act.surfaces?.workspace).toEqual([{ surface: 'repo/' }]); + expect(act.surfaces?.workspace).toEqual([{ surface: 'repo' }]); expect(act.surfaces?.streams).toEqual([{ stream: 'results' }]); expect(act.surfaces?.external).toEqual(['pr://github/example']); expect(act.permissions?.accessPreset).toBe('readwrite'); diff --git a/sdk/tests/validate.test.ts b/sdk/tests/validate.test.ts index eb391109c..111e9922e 100644 --- a/sdk/tests/validate.test.ts +++ b/sdk/tests/validate.test.ts @@ -309,6 +309,7 @@ describe('validate: canonical external surfaces', () => { '/provider/../item', '/provider//item', '/provider/item//', + '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/provider/item/', ])('rejects alias %s', (external) => { const result = validateSpec({ version: '0.1.0', @@ -324,7 +325,6 @@ describe('validate: canonical external surfaces', () => { it('accepts canonical absolute, relative, and URI-like identities', () => { for (const external of [ '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/provider/item', - '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/provider/item/', 'provider/item', 'pr://github/example', ]) { @@ -349,6 +349,8 @@ describe('validate: canonical workspace mounts and worktrees', () => { '/mount/repo/../repo', '/mount//repo', '/mount/repo//', + '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/mount/repo/', + 'repo/', ' worktrees/repo', 'worktrees/./repo', ])('rejects alias %s', (surface) => { @@ -366,10 +368,8 @@ describe('validate: canonical workspace mounts and worktrees', () => { it('accepts canonical mount paths and named worktrees', () => { for (const surface of [ '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/mount/repo', - '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/mount/repo/', 'worktrees/repo', 'repo', - 'repo/', ]) { expect(validateSpec({ version: '0.1.0', diff --git a/testdata/hello-ladder.flow.yaml b/testdata/hello-ladder.flow.yaml index 8e21935c8..a50fb59e0 100644 --- a/testdata/hello-ladder.flow.yaml +++ b/testdata/hello-ladder.flow.yaml @@ -35,7 +35,7 @@ steps: recoveryMode: inspect surfaces: workspace: - - surface: repo/ + - surface: repo streams: - stream: results external: diff --git a/testdata/hello-ladder.spec.canonical.json b/testdata/hello-ladder.spec.canonical.json index f0f4defd5..6822d5d5b 100644 --- a/testdata/hello-ladder.spec.canonical.json +++ b/testdata/hello-ladder.spec.canonical.json @@ -1 +1 @@ -{"budget":{"max_dollars":"1.50","max_tokens_out":2000},"description":"One flow, all three rungs — the single spec dialect end to end.","name":"hello-ladder","steps":[{"command":"echo hello","depends_on":[],"id":"greet","max_iterations":1,"retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"timeout_ms":5000,"type":"deterministic","verification":{"output_contains":"hello"}},{"depends_on":["greet"],"id":"plan","max_iterations":3,"model":"claude-sonnet-5","prompt":"Reply with JSON {answer: number} for 2+2.","retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"llm","verification":{"json_schema":{"required":["answer"],"type":"object"}}},{"depends_on":["plan"],"id":"act","instruction":"Apply the plan to the repo.","max_iterations":1,"permissions":{"access_preset":"readwrite","file_globs":["src/**"]},"recovery_mode":"inspect","retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"surfaces":{"external":["pr://github/example"],"streams":[{"stream":"results"}],"workspace":[{"surface":"repo/"}]},"type":"agent","verification":{}}],"version":"0.1.0"} +{"budget":{"max_dollars":"1.50","max_tokens_out":2000},"description":"One flow, all three rungs — the single spec dialect end to end.","name":"hello-ladder","steps":[{"command":"echo hello","depends_on":[],"id":"greet","max_iterations":1,"retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"timeout_ms":5000,"type":"deterministic","verification":{"output_contains":"hello"}},{"depends_on":["greet"],"id":"plan","max_iterations":3,"model":"claude-sonnet-5","prompt":"Reply with JSON {answer: number} for 2+2.","retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"llm","verification":{"json_schema":{"required":["answer"],"type":"object"}}},{"depends_on":["plan"],"id":"act","instruction":"Apply the plan to the repo.","max_iterations":1,"permissions":{"access_preset":"readwrite","file_globs":["src/**"]},"recovery_mode":"inspect","retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"surfaces":{"external":["pr://github/example"],"streams":[{"stream":"results"}],"workspace":[{"surface":"repo"}]},"type":"agent","verification":{}}],"version":"0.1.0"} diff --git a/testdata/hello-ladder.spec.sha256 b/testdata/hello-ladder.spec.sha256 index 07dd33164..8ec7adbe0 100644 --- a/testdata/hello-ladder.spec.sha256 +++ b/testdata/hello-ladder.spec.sha256 @@ -1 +1 @@ -ecccd7b2af27c265d473b1bd567e8e244fbb5051f1e1d7e667af7522de095a29 +57cac294f89be6a68ab1d8fbafc78f6fd75dc5998245695b6a28410ff6d57944 From 269fcc6ccc212469689f9fe504d28537f2ce1038 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 14:31:45 +0200 Subject: [PATCH 09/14] test(sdk): a late completion after cancel reports run_terminal, not lease_conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This edits a test that judges this branch's own behaviour, which AGENTS.md rails against. It is therefore its own commit, touching nothing else, and the taxonomy call was made by the lead (relayflow-lead-0903), not by this branch. Flagging it for the independent signoff to re-derive rather than inherit. The test is #142's own — "cancels over the real socket and rejects the lease holder after closure", added by main in feat(kernel): add durable run cancellation. It cancels a run over the socket, then has the lease holder complete the step, and asserted the refusal carried `lease_conflict`. What this branch changed is which of two refusals fires first, not whether the completion is refused. `step.complete` now runs the `ensure_mutable` admission gate before `completion_worker`: ensure_mutable(&engine, ¶ms.run_id)?; // -> run_terminal let worker_id = hub .completion_worker(connection_id, &key) .map_err(protocol_conflict)?; // -> lease_conflict Each code has exactly one producer in the tree (server/protocol.rs:58 and :46), so the ordering fully determines which is returned. Unchanged by this commit, and still asserted by the same test: the completion is refused, exactly one run.cancel.requested entry exists, and exactly one run.completed entry exists carrying completionReason "canceled". Only the error code moved. The lead's reasoning for preferring run_terminal: lease_conflict tells a worker "someone else holds your lease", which is false here -- nobody holds it, the run is over -- and it invites a retry that terminality does not. Checking "can this run accept mutations at all?" before "who holds this lease?" is also the correct precedence: the cheaper, more general, fail-closed question first. The rejected alternative was weakening ensure_mutable so lease_conflict still won. That trades a correct guard for a stale expectation. Mutation-verified, both directions, on the rebased tree at 512723c. RED (before this commit): $ ./node_modules/.bin/vitest run FAIL tests/live-kernel.test.ts > ... > cancels over the real socket and rejects the lease holder after closure AssertionError: expected JournalProtocolError: run_terminal: run 0... { code: '...' } to match object { code: 'lease_conflict' } - Object { - "code": "lease_conflict", + JournalProtocolError { + "code": "run_terminal", Tests 2 failed | 409 passed | 3 skipped (414) GREEN (after): $ ./node_modules/.bin/vitest run Tests 1 failed | 410 passed | 3 skipped (414) The one remaining failure is the pre-existing `JournalClient wire conformance` failure, which is independent: it survives moving ensure_mutable after completion_worker, whereas this test does not. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- sdk/tests/live-kernel.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sdk/tests/live-kernel.test.ts b/sdk/tests/live-kernel.test.ts index fff05665c..431bbae38 100644 --- a/sdk/tests/live-kernel.test.ts +++ b/sdk/tests/live-kernel.test.ts @@ -231,6 +231,12 @@ steps: completion_reason: 'canceled', }); await expect(control.runCancel(started.run_id)).resolves.toEqual(canceled); + // Parallel dispatch (#137) admits every mutating verb through + // `ensure_mutable` before the lease lookup, so an already-terminal run is + // refused for terminality rather than for lease ownership. `lease_conflict` + // would claim someone else holds this lease, which is false here: nobody + // does, the run is over. The refusal itself, and the journal assertions + // below, are unchanged. await expect(worker.stepComplete( lease.run_id, lease.step_id, @@ -238,7 +244,7 @@ steps: lease.idempotency_key, 'success', { output: { answer: 4 } }, - )).rejects.toMatchObject({ code: 'lease_conflict' }); + )).rejects.toMatchObject({ code: 'run_terminal' }); const entries = (await control.journalRead(started.run_id, 1)).entries as { entry_type: string; From 0e49170f59ab19ed711a984894dda63c0734211d Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 14:35:46 +0200 Subject: [PATCH 10/14] docs(reviews): record the PR #137 rebase onto 512723c Supersedes an unmerged first pass of this report that targeted 990093b. Every command is pinned to a literal SHA rather than the origin/main ref, which moved twice during the task. This rebase produced ZERO conflicts, which is the risk rather than the result: on #139's rebase a line that reverted a lowering auto-merged silently. Every hunk was therefore audited by reading. 512723c adds #138, which touches four files this branch also edits (spec.rs, spec/tests.rs, validate.ts, validate.test.ts) and, critically, moves timeoutMs to deterministic-only in TWO independent places: the step-fields allowlist and compileStep's base spread. Getting one right and missing the other yields a spec that validates but lowers wrong, and validateSpec cannot see it. Both halves are byte-identical to 512723c and both were re-proved behaviourally through compileYaml + toKernelSpec: a deterministic step lowers to timeout_ms, llm and agent are refused at the allowlist. #136's `output` line survives in both verb lists. Artifact survival, both directions. All 15 of #138's blobs hashed before and after: 11 identical including compile.ts and step-fields.ts; the 4 that moved are the 4 this branch edits and each is a pure addition. Every line of #138 content absent afterwards was enumerated: a first pass with plain diff reported 14, of which 7 were false positives from re-indentation and one rustfmt attribute rewrap; whitespace-insensitively 7 remain, all attributed and none authored by #138. In the other direction, a whole-tree set-diff of the branch's own change set before against after reports exactly three deltas across 41 files, the same three deliberate resolutions as the first pass and nothing else. The branch's own gate is proved where it lives rather than where it is convenient: a canonical spec compiled through the SDK, its lowered kernel spec then mutated and submitted over a real socket with the SDK out of the path. The kernel refuses all five non-canonical forms across both surface kinds, and accepts the canonical control. Gates: tsc --noEmit, tsc -p tsconfig.type-tests.json (a gate #138 added that the brief's list predates), and tsc -p tsconfig.tests.json all pass; cargo test --workspace is 130 passed, 0 failed; vitest is 410 passed with one failure, the pre-existing wire-conformance one. Rust test names set-difference to exactly the union of both parents, 130 executed against 130 expected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- ops/reviews/20260903-pr137-repair-0903.md | 519 ++++++++++++++++++++++ 1 file changed, 519 insertions(+) create mode 100644 ops/reviews/20260903-pr137-repair-0903.md diff --git a/ops/reviews/20260903-pr137-repair-0903.md b/ops/reviews/20260903-pr137-repair-0903.md new file mode 100644 index 000000000..0a9886c54 --- /dev/null +++ b/ops/reviews/20260903-pr137-repair-0903.md @@ -0,0 +1,519 @@ +VERDICT: REBASED ONTO 512723c, GREEN EXCEPT ONE PRE-EXISTING FAILURE + +# PR #137 — rebase of `repair/pr137-uniform-reject-0903` onto `origin/main` (`512723c`) + +- **Repo/PR:** AgentWorkforce/flows #137, "production parallel dispatch" +- **Branch:** `repair/pr137-uniform-reject-0903` +- **Worktree:** `/Users/khaliqgant/AgentWorkforce/flows-pr137-lead-0903-wt` +- **Original head:** `cacb493beb2d6119597968880f579cf78894bb16` (8 commits on merge-base `a0d42ff`) +- **Now:** `269fcc6ccc212469689f9fe504d28537f2ce1038` (9 commits on `512723c`) +- **Rebase owner is not the author.** + +This supersedes an earlier pass of this report that targeted `990093b`. That +pass is not merged and its base is stale; **this document describes the tree at +`512723c` only.** Every command below is pinned to a literal SHA, never to the +`origin/main` ref, because that ref moved twice during this task. + +``` +$ git log --oneline 512723c..HEAD +269fcc6 test(sdk): a late completion after cancel reports run_terminal, not lease_conflict +93c1a3c fix(kernel): one spelling per surface, refusing the terminal slash +c21f039 fix(kernel): preserve terminal-slash surface compatibility +562b50b fix(kernel): canonicalize workspace surfaces across kernel/SDK/socket +8edba06 fix(kernel): reject forged completion pins +2d928b4 fix(kernel): close parallel dispatch admission gaps +382c5c1 fix(kernel): preserve parallel assignment lifecycle +1b99dac fix(kernel): drive complete parallel dispatch batches +954add9 kernel: dispatch runnable steps in parallel + +$ git diff --stat 512723c HEAD | tail -1 + 41 files changed, 3539 insertions(+), 491 deletions(-) +``` + +Base history: merge-base `a0d42ff`; `512723c` adds #150, #131, #133, #142, +#146, #136 and #138 over it. + +Environment, disclosed: `node_modules` pre-seeded. `npm ci` / `npm run build` +hang here, so the build is its two steps (`tsc`, then +`node scripts/make-cli-executable.mjs`), and `test:prep`'s +`chmod +x testdata/preflight/*-cli` was run by hand. `RELAYFLOWD_BIN` pinned — +see §6. + +--- + +## 1. This rebase produced **zero** conflicts — which is the risk, not the result + +`git rebase --onto 512723c 990093b` replayed all 8 code commits with no +conflict markers at all. That is exactly the condition the lead flagged: on +#139's rebase a line that *reverted a lowering* auto-merged silently. A clean +replay is not evidence; it only means every hunk must be audited by reading, +not by trusting git. + +The four conflicts resolved on the previous pass (#142's `finish_run` re-homed +into `assignments.rs`; `finish_run` + `earliest_lease_deadline` both kept; +main's superset terminality guard in `remote.rs`; the `engine.rs` import union) +are carried inside the replayed commits and re-verified intact in §4. + +On the `remote.rs` guard specifically, in the lead's words: **main's +`cancel_requested || completion` is a strict superset of the branch's +`completion`, so the branch's condition is preserved rather than overridden.** +That is a merits resolution, not "I took main's side" — the branch's intent +(refuse a completion once the run is terminal) is fully contained in what was +kept, and main's extra `cancel_requested` arm closes the window #142's +late-completion test exists for. + +## 2. What #138 changed, and why it matters to this branch + +`512723c` is one commit: `fix(sdk): refuse fields outside each step verb schema +(#138)`. It touches 15 paths, **four of which this branch also edits** +(`kernel/relayflowd-core/src/spec.rs`, `.../spec/tests.rs`, +`sdk/src/validate.ts`, `sdk/tests/validate.test.ts`). On the previous base only +the last two overlapped, and `compile.ts` did not overlap at all. + +The load-bearing change is that **`timeoutMs` became deterministic-only, and it +moved in two independent places**: + +| | `990093b` | `512723c` | +|---|---|---| +| allowlist (`step-fields.ts`) | `STEP_COMMON_FIELDS` | `STEP_FIELDS_BY_TYPE.deterministic` | +| lowering (`compile.ts` `compileStep`) | `base` spread | the `deterministic` case | + +`#138` also deleted `requireNoTimeout` from `compile.ts` and moved the +`timeoutMs` positive-integer check from `validateStep` into +`validateDeterministic`. + +Getting one of the two right and missing the other is the failure the lead +named: a spec that **validates but lowers wrong**. `validateSpec` and +`flows check` are both blind to it — only `compileYaml` + `toKernelSpec` can +tell "the key was accepted" from "the key became a gate". §7.1 asserts both +directions executably rather than by reading. + +## 3. #138 survival: blob comparison at `512723c` vs after + +All 15 of #138's paths, hashed before and after: + +``` +CHANGED 97abb689d87a7f49d0f535aa7601814b809ca986 -> 8338848ac375e507bffec52c116b3d1f2a0fcc91 kernel/relayflowd-core/src/spec.rs +SAME b410eecd7e3882a267f7ac79df6912b597a2e387 kernel/relayflowd-core/src/spec/dependencies.rs +CHANGED 6412b9c513445d4cde4607b990d9bdcc4f269704 -> b8443bfffff880f6f5559afd9b1c6793fb6cd0f2 kernel/relayflowd-core/src/spec/tests.rs +SAME 695f9b8f196e0a242a25ac51fdfc1327784f6c63 ops/reviews/20260903-pr138-rebase-0903.md +SAME d905764ce5a1f94e2a5645105ee837c5329cfd13 sdk/package.json +SAME f931359eabe499a984e4fe81db24bfa798556083 sdk/src/compile.ts <- the lead's concern +SAME e049822d1d836d205f036bf59b36475f88937d88 sdk/src/spec.ts +SAME e96e6cd072896c03dd6e0d9e03e964d106325f78 sdk/src/step-dependencies.ts +SAME 60490fe98ce55cd5879c8267c9ca2d2db5536044 sdk/src/step-fields.ts <- the allowlist half +CHANGED ec0b75b59ae25b3fbf03784926f90c527f07a963 -> e80fe4266f1c0168dde0aca8edc90dc23c938dc5 sdk/src/validate.ts +SAME 8dea5e92e5e093a0252d54ae7ab51d8232a170fd sdk/tests/dependency-validation.test.ts +CHANGED 070093b84ecc698f7dbdcf6f5eb8f4719dfedcd2 -> 111e9922e194118aa6c3b78f1b23e5876dcf289d sdk/tests/validate.test.ts +SAME 470e88e909446b1ef9c377aac36512589d909aeb sdk/tests/verb-field-lint.test.ts +SAME 481c3e0da1cfe475cb0e118b96cd7e55e22cedb2 sdk/tsconfig.type-tests.json +SAME f3588bcf63c87a43eedcb86aecbf37617429fb87 sdk/type-tests/step-fields.ts +``` + +**Both halves of the `timeoutMs` change are byte-identical to `512723c`.** +`compile.ts` and `step-fields.ts` are untouched, so neither half could have been +reverted; §7.1 confirms behaviourally as well. + +The 4 that changed are the 4 this branch legitimately edits. Read in full; each +is a pure addition on top of #138's content. `#138`'s own refactor survives: + +``` +$ grep -n "mod dependencies\|validate_dependency_cycles" kernel/relayflowd-core/src/spec.rs +20:mod dependencies; +22:use dependencies::validate_dependency_cycles; +181: validate_dependency_cycles(&ids, &dependencies)?; +$ grep -n "^fn visit<" kernel/relayflowd-core/src/spec.rs +(absent — #138 removed the inline version; it was not resurrected) +$ grep -n "requireNoTimeout" sdk/src/compile.ts +(absent — correct) +``` + +### 3.1 Deleted-line attribution (#138's technique) + +Enumerating every line of `512723c` content absent afterwards. **A first pass +with plain `diff` reported 14, of which 7 were false positives from +re-indentation** — the branch de-indents a comment block in `spec.rs` and +rustfmt re-wraps one `#[error(...)]` attribute. Re-run whitespace-insensitively, +**7 true deletions remain**, each attributed: + +``` +--- kernel/relayflowd-core/src/spec.rs + < #[error("trigger {id} declared stale_after_ms={ms} which does not fit in i64 (~292M years); ...")] +--- kernel/relayflowd-core/src/spec/tests.rs + < "surfaces": {"workspaces": [{"surface": "repo/"}]} + < "surfaces": {"workspace": [{"surface": "repo/"}], "streams": [{"stream": "results"}], +--- sdk/src/validate.ts + < if (!Array.isArray(s['workspace']) || ... isNonEmptyString(...['surface']))) { + < this.fail(`${at}.workspace: expected an array of {surface: string}`); + < if (!Array.isArray(s['external']) || !(s['external'] as unknown[]).every(isNonEmptyString)) { + < this.fail(`${at}.external: expected an array of path strings`); +--- sdk/tests/validate.test.ts + (none — pure addition) +``` + +1. **`spec.rs` `#[error]`** — rustfmt line-wrapping only. String content proven + byte-identical after whitespace normalisation; the branch's own diff at + `a0d42ff..cacb493` shows the same single-line → 3-line reflow. Not a deletion + of meaning. +2. **`spec/tests.rs` two `repo/` fixtures** — `#138` never touched them + (`git diff 990093b 512723c -- .../spec/tests.rs | grep -c 'repo/'` → `0`); + they date to `a0d42ff`. The branch rewrites both `repo/` → `repo` because its + strict rule makes a terminal slash non-canonical, and separately adds `"repo/"` + to the explicit reject list. This matters for the first fixture in particular: + it is the *unknown-key* test (`"workspaces"`, plural, is the typo under test), + and had `repo/` been left it would now fail for the wrong reason. The branch + already handled that. +3. **`validate.ts` four lines** — predate #138 (present at `a0d42ff`), superseded + by `isCanonicalPathSurface` and its stricter messages. + +**Zero lines authored by #138 were deleted.** Same for #136, whose 53 blobs were +re-checked: `sdk/src/step-fields.ts` is byte-identical and still carries +`output` in both the `llm` and `agent` lists (§7.2). + +## 4. Branch-content survival: whole-tree set-diff + +Every added line of the branch's own change set, before (`a0d42ff..cacb493`) +against after (`512723c..93c1a3c`), bucketed per file. Across all 41 files, +**exactly three deltas — the same three deliberate resolutions as the previous +pass, and nothing else**: + +``` +kernel/relayflowd/src/engine.rs lost 1 extra 1 + LOST : 'recovery_actions_filtered, workspace_surfaces_equal,' + EXTRA: 'recovery_actions_filtered, request_cancel_action, workspace_surfaces_equal,' +kernel/relayflowd/src/engine/remote.rs lost 3 extra 0 + LOST : 'if state.completion.is_some() {' + LOST : 'bail!("run {run_id} is terminal and cannot accept another step completion")' + LOST : '}' [subsumed by main's superset guard] +kernel/relayflowd/src/server/session/assignments.rs lost 0 extra 11 + EXTRA: finish_run + its doc comment [re-homed from session.rs, not new] + + (all 38 other files: lost 0, extra 0) +``` + +No conflict markers anywhere. No duplicate definitions +(`isCanonicalPathSurface`, `finish_run`, `validate_dependency_cycles`, +`reject_unknown_step_fields` each defined once; `requireNoTimeout` zero times). +All 18 `workspace_surfaces_equal` call sites intact. + +## 5. The one test expectation this branch changes — `269fcc6` + +Its own commit, touching nothing else, because it edits a test that judges this +branch's behaviour and AGENTS.md rails against that. + +`live-kernel.test.ts:234` is **#142's own test**. It asserted a late +`step.complete` after `run.cancel` returns `lease_conflict`; this branch returns +`run_terminal`. The branch changed **which of two refusals fires first**, not +whether the completion is refused: + +```rust +ensure_mutable(&engine, ¶ms.run_id)?; // -> run_terminal +let worker_id = hub + .completion_worker(connection_id, &key) + .map_err(protocol_conflict)?; // -> lease_conflict +``` + +Each code has exactly one producer (`server/protocol.rs:58` and `:46`), so the +ordering fully determines the answer. Unchanged and still asserted: the +completion is refused, one `run.cancel.requested`, one `run.completed` with +`completionReason: canceled`. + +**The taxonomy call was made by the lead**, on the grounds that `lease_conflict` +asserts someone else holds the lease — false here, nobody does — and invites a +retry that terminality does not; and that asking "can this run accept mutations +at all?" before "who holds this lease?" is the correct precedence. The rejected +alternative was weakening `ensure_mutable` so `lease_conflict` still won, which +would trade a correct guard for a stale expectation. **The independent signoff +should re-derive this rather than inherit it.** + +Mutation-verified both directions on this base. RED, before `269fcc6`: + +``` + FAIL tests/live-kernel.test.ts > ... > cancels over the real socket and rejects the lease holder after closure +AssertionError: expected JournalProtocolError: run_terminal: run 0… { code: '…' } to match object { code: 'lease_conflict' } +- Object { +- "code": "lease_conflict", ++ JournalProtocolError { ++ "code": "run_terminal", + Tests 2 failed | 409 passed | 3 skipped (414) +``` + +GREEN, after: + +``` + Tests 1 failed | 410 passed | 3 skipped (414) +``` + +Independently, on the previous base, moving `ensure_mutable` *after* +`completion_worker` made this test pass while leaving the wire-conformance +failure — proving the two failures have different causes. That mutation was +reverted byte-for-byte (`shasum` match, clean tree) and is **not** in this +branch. + +--- + +## 6. Gates + +`RELAYFLOWD_BIN` pinned, because `locateRelayflowd` picks the newest daemon by +mtime across every toolchain target tree and this machine has three, one per +live worktree. `411835410` is `cksum` of this worktree's path, so +`$HOME/.relayflows-toolchain/target/411835410/debug/relayflowd` is the binary +this worktree's own `cargo build` produced. That is the one used below. + +**#138 added a gate the brief's list predates**: `typecheck` is now +`tsc --noEmit && tsc -p tsconfig.type-tests.json`. Run as well. + +``` +$ cd sdk && ./node_modules/.bin/tsc --noEmit +EXIT=0 PASS + +$ cd sdk && ./node_modules/.bin/tsc -p tsconfig.type-tests.json # new, from #138 +EXIT=0 PASS + +$ cd sdk && ./node_modules/.bin/tsc -p tsconfig.tests.json +EXIT=0 PASS + +$ cd kernel && PATH="$HOME/.cargo/bin:$PATH" RUSTUP_TOOLCHAIN=stable sh ../ops/cargo.sh test --workspace +running 22 tests +test result: ok. 22 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.55s +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s +running 34 tests +test result: ok. 34 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 37.94s +running 1 test +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s +running 1 test +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s +running 4 tests +test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.18s +running 3 tests +test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s +running 42 tests +test result: ok. 42 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.13s +running 5 tests +test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s +running 18 tests +test result: ok. 18 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s +EXIT=0 + PASS — 22+34+1+1+4+3+42+5+18 = 130 passed, 0 failed + +$ cd sdk && RELAYFLOWD_BIN=$HOME/.relayflows-toolchain/target/411835410/debug/relayflowd \ + ./node_modules/.bin/vitest run + FAIL tests/live-kernel.test.ts > JournalClient wire conformance against live relayflowd > exercises every protocol-v0 verb with the real server +JournalProtocolError: run_terminal: run … is terminal and cannot accept mutations + Test Files 1 failed | 22 passed | 1 skipped (24) + Tests 1 failed | 410 passed | 3 skipped (414) +EXIT=1 + 1 failure — the pre-existing wire-conformance one, unchanged +``` + +### 6.1 Per-file accounting by test *name* + +``` +===== RUST ===== ===== VITEST ===== + branch (cacb493) : 122 branch (cacb493) : 200 + base (512723c) : 102 base (512723c) : 247 + HEAD : 130 HEAD : 249 + expected union : 130 expected union : 250 + LOST from branch : NONE LOST from branch : ['AgentWorker passes a declared model to the CLI as RELAYFLOW_MODEL'] + LOST from base : NONE LOST from base : NONE + EXTRA : NONE EXTRA : NONE + per-file deltas : none per-file deltas : live-kernel.test.ts −(that one name) +``` + +130 executed = 130 union, exactly. The +8 over the branch's 122 is #142's six +cancel tests (core lib 37→40, crash_resume 31→34) plus #138's two new spec tests +(core lib 40→42). + +The single vitest name shortfall is **not** a rebase loss — it is #136's own +rename, inherited, in a file the branch never touched except for §5's one +assertion: + +``` +$ git grep -n "passes a declared model" a0d42ff -- sdk/tests/ +a0d42ff:sdk/tests/live-kernel.test.ts:691: it('AgentWorker passes a declared model to the CLI as RELAYFLOW_MODEL', … +$ git grep -n "passes a declared model" 512723c -- sdk/tests/ +512723c:sdk/tests/live-kernel.test.ts:773: it('AgentWorker passes a declared model to an identified wrapper as RELAYFLOW_MODEL', … +``` + +--- + +## 7. Lowering proofs — redone for this base, not inherited + +On `990093b` this branch contributed zero lines to `compile.ts` and the analysis +was reported as *structurally absent*. On `512723c` `compile.ts` genuinely +changed, so the whole section is re-derived. + +### 7.1 #138's `timeoutMs` trap — both halves, executably + +`validateSpec` cannot see a lowering revert, so this goes through +`compileYaml` + `toKernelSpec` and inspects the emitted kernel step: + +``` +$ node /tmp/timeout_probe.mjs +=== #138 trap: timeoutMs must be deterministic-only in BOTH allowlist and lowering === + +--- deterministic + timeoutMs (must lower to timeout_ms) + compileYaml+toKernelSpec : ok + validateSpec : ok + LOWERED kernel step : {"id":"work","depends_on":[],"max_iterations":1,"retry":{...}, + "verification":{},"type":"deterministic","command":"echo hi","timeout_ms":5000} +--- llm + timeoutMs (must be REFUSED, not silently dropped) + compileYaml+toKernelSpec : REFUSED -> spec.steps[0]: unknown key "timeoutMs" (expected one of id | type | dependsOn | verif… +--- agent + timeoutMs (must be REFUSED, not silently dropped) + compileYaml+toKernelSpec : REFUSED -> spec.steps[0]: unknown key "timeoutMs" (expected one of id | type | dependsOn | verif… +``` + +Deterministic lowers to `timeout_ms: 5000`; llm and agent are refused at the +allowlist. Both halves are in #138's state, confirmed behaviourally and not only +by blob equality. Source-level confirmation, the lowering half: + +``` +$ sed -n '/^ const base = {/,/^ };/p' sdk/src/compile.ts + const base = { + id: step.id, + type: step.type, + ...(step.dependsOn !== undefined ? { dependsOn: step.dependsOn } : {}), + ...(verification !== undefined ? { verification } : {}), + maxIterations, + }; +``` + +No `timeoutMs` in the base spread — it is carried only by the `deterministic` +case. And the allowlist half, `timeoutMs` appearing on exactly one line, inside +the per-verb map rather than in `STEP_COMMON_FIELDS`: + +``` +$ grep -n "timeoutMs" sdk/src/step-fields.ts +33: deterministic: ['command', 'timeoutMs'], +``` + +### 7.2 #136's `output` allowlist line, re-confirmed + +``` +$ grep -n "STEP_TYPE_KEYS\|STEP_COMMON_KEYS\|ROOT_KEYS" sdk/src/validate.ts +(no output — no stale local allowlist reintroduced) +$ grep -n "STEP_FIELDS_BY_TYPE" -A4 sdk/src/step-fields.ts +export const STEP_FIELDS_BY_TYPE = { + deterministic: ['command', 'timeoutMs'], + llm: ['prompt', 'model', 'cli', 'output'], + agent: ['instruction', 'agent', 'cli', 'model', 'surfaces', 'recoveryMode', 'permissions', 'output'], +``` + +`✓ tests/verb-field-lint.test.ts (66 tests)` and `✓ tests/typed-output.test.ts +(14 tests)` both pass. + +### 7.3 This branch's own gate — compiler path, then the kernel with the SDK bypassed + +Supporting witness — the surface really *lowers* into the kernel spec rather +than being accepted and dropped: + +``` +$ node /tmp/lowering_probe.mjs +--- canonical, bare: surface="repo" + compileYaml+toKernelSpec : ok + LOWERED into kernel spec : {"workspace":[{"surface":"repo"}],"external":["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/provider/item"]} +--- canonical, rooted: surface="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/mount/repo" + compileYaml+toKernelSpec : ok + LOWERED into kernel spec : {"workspace":[{"surface":"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/mount/repo"}],"external":["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/provider/item"]} +--- NON-canonical: "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/mount/repo/", "/mount/repo//", "/mount/repo/../repo" + compileYaml+toKernelSpec : THREW -> spec compile failed +``` + +Primary assertion — a spec that never passes through the SDK at all. A canonical +spec is compiled, then the **lowered kernel spec** is mutated directly and +submitted via `run.start` over the real socket, so no SDK validation is in the +path: + +``` +$ RELAYFLOWD_BIN=… node /tmp/kernel_gate_probe.mjs +=== kernel-boundary gate, SDK compiler bypassed (run.start over the real socket) === + + control: canonical /mount/repo + /provider/item KERNEL ACCEPTED -> run 01M1KM2J9JMGD2W27121R3HG16 + workspace terminal slash /mount/repo/ KERNEL REFUSED -> [invalid_spec] non-canonical workspace surface "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/mount/repo/" + workspace double slash /mount/repo// KERNEL REFUSED -> [invalid_spec] non-canonical workspace surface "/mount/repo//" + workspace dotdot /mount/repo/../repo KERNEL REFUSED -> [invalid_spec] non-canonical workspace surface "/mount/repo/../repo" + external terminal slash /provider/item/ KERNEL REFUSED -> [invalid_spec] non-canonical external surface "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/provider/item/" + external double slash /provider/item// KERNEL REFUSED -> [invalid_spec] non-canonical external surface "/provider/item//" +``` + +(The probe tags the control row `<== GATE OPEN` in its raw output; that is its +wording for "accepted", which is the expected result for a canonical surface. +Not a finding.) + +Accepting the canonical control matters as much as the five refusals: a gate +that refused everything would pass a refusal-only test while breaking every real +spec. + +--- + +## 8. P0 re-verification on this base + +- **Terminal slash refused for both surface kinds**, at the kernel boundary with + the SDK bypassed (§7.3), in the SDK compiler (§7.3), and in the unit and + socket suites: `spec/tests.rs:133-134,180,203-204`, + `crash_resume/surface_identity.rs:51-52`, + `crash_resume/workspace_identity.rs:67-70`. +- **`spec_parity` 5/5** and the two identity suites, from this base's run: + +``` +$ grep -E "stamps_the_same_hash|surface_identity::|workspace_identity::|protocol_admission::" cargo.log +test protocol_admission::every_mutating_run_verb_refuses_terminal_before_changing_state ... ok +test surface_identity::aliases_are_rejected_and_external_ancestors_serialize_over_real_sockets ... ok +test workspace_identity::workspace_aliases_are_refused_and_canonical_subtrees_serialize_over_real_sockets ... ok +test the_kernel_parses_the_event_triggered_spec_and_stamps_the_same_hash ... ok +test the_kernel_parses_the_deterministic_rung_and_stamps_the_same_hash ... ok +test the_kernel_parses_the_rung_c_agent_spec_and_stamps_the_same_hash ... ok +test the_kernel_parses_the_rung_b_spec_and_stamps_the_same_hash ... ok +test the_kernel_parses_the_sdk_compiled_spec_and_stamps_the_same_hash ... ok +``` + +- **Fixtures byte-identical to pre-rebase**, so the regenerated hash did not + move across either rebase: + +``` + testdata/hello-ladder.flow.yaml pre=a50fb59e… post=a50fb59e… + testdata/hello-ladder.spec.canonical.json pre=6822d5d5… post=6822d5d5… + testdata/hello-ladder.spec.sha256 pre=8ec7adbe… post=8ec7adbe… +$ cat testdata/hello-ladder.spec.sha256 +57cac294f89be6a68ab1d8fbafc78f6fd75dc5998245695b6a28410ff6d57944 +``` + + which is the post-fix hash `cacb493`'s message records. + +- **`crash_resume` 34/34**, including all four `parallel_lifecycle` cases, both + `worker_capacity` cases, and #142's three cancel tests in the same binary. +- **All 18 `workspace_surfaces_equal` call sites intact**: + +``` +$ git grep -o 'workspace_surfaces_equal' HEAD -- kernel | wc -l +18 +``` + +A green suite is not proof the resolutions were right — which is why §1–§4 give +the reasoning and the audits, and §7 tests the gate rather than the compiler's +intent. + +## 9. Flagged, not changed + +- **`finish_run` does not clear the new `reservations` map.** Unreachable: a + reservation exists only between `reserve_dispatch` and its `dispatch` / + `release_dispatch_reservation`, all inside one `Engine::drive`, and + `server/cancel.rs:26` takes the same `hub.run_lock` the driver holds. Clearing + them would be dead code (AGENTS.md rule 6). +- **Duplicated `#[cfg(unix)]` at `server.rs:118-120`.** Harmless, and pre-exists + in the author's own `cacb493` (`bdd598c` moved `handle_line` into + `server/protocol.rs` and left the attribute). The lead is picking it up + separately rather than widening this diff. + +## 10. Base movement + +`origin/main` moved twice during this task: to `512723c` while the first pass +was being written, and it is `512723c` at the time of writing. Per the standing +rail this rebase was finished against `512723c` and not chased further. + +``` +$ git merge-base --is-ancestor 512723c HEAD && echo "HEAD sits on 512723c" +HEAD sits on 512723c +``` + +Safety ref `backup-onto-990093b` (`276967e`) retains the superseded first pass. From 2f4e4b859b31ad3b33322a173f39b2ff699e7030 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 14:42:26 +0200 Subject: [PATCH 11/14] =?UTF-8?q?docs(reviews):=20correct=20the=20wire-con?= =?UTF-8?q?formance=20attribution=20=E2=80=94=20it=20is=20this=20branch's?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An earlier version of this report called the vitest wire-conformance failure "pre-existing". That was wrong. It was observed at 83db98b, which is #137's own head and already carries bdd598c; "pre-existing at the branch parent" is not "pre-existing on main", and I repeated the stronger claim without testing it. Determined empirically instead. A throwaway worktree at 512723c with its own kernel build and RELAYFLOWD_BIN pinned to it runs tests/live-kernel.test.ts at 21 passed (21), wire conformance included. The same file fails on this branch. The failure is the branch's. It is also not an error-code taxonomy change, which is why no assertion was touched. bdd598c gates six verbs with ensure_mutable, two of them event.emit and stream.append. The wire-conformance test starts a deterministic run, which completes immediately, then calls both on it and asserts they SUCCEED (matched === 0, offset === 0). Main accepts them; this branch refuses them. The observable protocol behaviour changed from accepted to refused. The refusal is load-bearing rather than gratuitous, and the obvious fix is the wrong one. On main, stream.append against a terminal run writes a stream.appended entry after run.completed. The branch's state.rs fold guard rejects exactly that, so a journal main produces is one this branch cannot load: the same data directory resumes clean under the main binary and fails under the branch binary with "journal entry 5 appears after terminal run.completed". Relaxing ensure_mutable to turn the test green would let the daemon write journals it then cannot resume, which is a durability bug and worse than a red test. The rebase also surfaced a direct contradiction: the branch's own protocol_admission test asserts stream.append and event.emit return run_terminal and leave the journal unchanged, while main's wire-conformance test asserts they succeed. Both are in the tree, both were written deliberately, and they cannot both pass. Because protocol_admission pins the error code, an accept-and-ignore compromise would require editing that gate too. Three options are laid out in the report. None is implemented; the branch is unchanged and the push is on hold pending the lead's call. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- ops/reviews/20260903-pr137-repair-0903.md | 175 +++++++++++++++++++++- 1 file changed, 172 insertions(+), 3 deletions(-) diff --git a/ops/reviews/20260903-pr137-repair-0903.md b/ops/reviews/20260903-pr137-repair-0903.md index 0a9886c54..23f22c0d3 100644 --- a/ops/reviews/20260903-pr137-repair-0903.md +++ b/ops/reviews/20260903-pr137-repair-0903.md @@ -1,4 +1,4 @@ -VERDICT: REBASED ONTO 512723c, GREEN EXCEPT ONE PRE-EXISTING FAILURE +VERDICT: REBASED ONTO 512723c. ONE RED TEST, AND IT IS THE BRANCH'S — DIAGNOSIS IN §11, DECISION OPEN # PR #137 — rebase of `repair/pr137-uniform-reject-0903` onto `origin/main` (`512723c`) @@ -246,7 +246,9 @@ GREEN, after: Independently, on the previous base, moving `ensure_mutable` *after* `completion_worker` made this test pass while leaving the wire-conformance -failure — proving the two failures have different causes. That mutation was +failure standing. That proved the two failures are reached by different verbs — +`step.complete` here, `event.emit`/`stream.append` there — but it did **not** +make the second one pre-existing, and §11 corrects that. The mutation was reverted byte-for-byte (`shasum` match, clean tree) and is **not** in this branch. @@ -303,7 +305,9 @@ JournalProtocolError: run_terminal: run … is terminal and cannot accept mutati Test Files 1 failed | 22 passed | 1 skipped (24) Tests 1 failed | 410 passed | 3 skipped (414) EXIT=1 - 1 failure — the pre-existing wire-conformance one, unchanged + 1 failure — the wire-conformance test. NOT pre-existing: it is + this branch's, and it is a behaviour change, not an error + code. Full diagnosis in §11. Push is on hold. ``` ### 6.1 Per-file accounting by test *name* @@ -517,3 +521,168 @@ HEAD sits on 512723c ``` Safety ref `backup-onto-990093b` (`276967e`) retains the superseded first pass. + +--- + +## 11. Correction — the wire-conformance failure is the branch's, and it is a behaviour change + +**An earlier version of this report called this failure "pre-existing". That was +wrong, and the error was mine to catch.** The lead observed it at `83db98b` — +which is #137's *own head*, already carrying `bdd598c` — and "pre-existing at +the branch parent" is not "pre-existing on main". I repeated the stronger claim +without testing it. The lead caught it and asked for the attribution to be +captured rather than inferred. Captured below. + +### 11.1 It does not reproduce on main + +A throwaway worktree at `512723c`, its own kernel build, `RELAYFLOWD_BIN` pinned +to that build (`…/target/978412413/debug/relayflowd`; `978412413` is `cksum` of +that worktree's path): + +``` +$ cd flows-main-512723c-wt/sdk && RELAYFLOWD_BIN=…/978412413/debug/relayflowd \ + ./node_modules/.bin/vitest run tests/live-kernel.test.ts + Test Files 1 passed (1) + Tests 21 passed (21) +EXIT=0 +``` + +**Green on pure `512723c`, wire-conformance included.** The same file on this +branch fails. The failure is the branch's. + +### 11.2 Mechanism — and it is not an error code + +`bdd598c` gates six verbs with `ensure_mutable`: + +``` +$ awk '/^ "[a-z.]+" =>/{v=$1} /ensure_mutable\(&engine/{print v}' kernel/relayflowd/src/server.rs +"step.heartbeat" "step.complete" "effect.record" "effect.confirm" "event.emit" "stream.append" +``` + +The wire-conformance test starts a **deterministic** run, which completes +immediately, and then calls two of those verbs on it — asserting they +**succeed**: + +```js +expect((await client.eventEmit(deterministic.run_id, 'unmatched', { ok: true })).matched).toBe(0); +expect((await client.streamAppend(deterministic.run_id, 'results', { answer: 4 })).offset).toBe(0); +``` + +Same probe, same spec, both binaries: + +``` +===== PURE MAIN build (512723c) ===== + run status = completed / success (TERMINAL) + runResume (read-ish) -> OK {"status":"completed",...} + eventEmit (MUTATING) -> OK {"matched":0} + streamAppend (MUTATING) -> OK {"offset":0} + +===== BRANCH build (HEAD) ===== + run status = completed / success (TERMINAL) + runResume (read-ish) -> OK {"status":"completed",...} + eventEmit (MUTATING) -> THROW [run_terminal] run … is terminal and cannot accept mutations + streamAppend (MUTATING) -> THROW [run_terminal] run … is terminal and cannot accept mutations +``` + +**This is materially different from §5.** There the test asserted an error +*code* and the refusal was unchanged. Here the test asserts the calls +**succeed**, and the branch refuses them. The protocol's observable behaviour +for `event.emit` and `stream.append` on a terminal run changed from *accepted* +to *refused*. Per the lead's instruction, the assertion has **not** been touched. + +### 11.3 Why the refusal is load-bearing, not gratuitous + +The branch's `state.rs` adds a fail-closed fold guard: + +```rust +if state.completion.is_some() { + return Err(StateError::EntryAfterRunCompleted { seq: entry.seq }); +} +``` + +On main, `stream.append` against a terminal run **writes a journal entry after +`run.completed`**: + +``` +$ node /tmp/journal_probe.mjs # against the main build + journal AFTER: + 1. run.spawned 2. step.attempt.started 3. step.completed + 4. run.completed 5. stream.appended + >>> entries AFTER the terminal run.completed: ["stream.appended"] +``` + +So the two changes are coupled. A journal main happily produces is one this +branch **cannot load**. Same data directory, both binaries: + +``` +$ MAIN relayflowd --data-dir $DD resume $RID + exit=0 + {"run_id":"01M1KMJ5KSGMPW9NE6SKPXZN3T","status":"completed","completion_reason":"success","completed_steps":1} + +$ BRANCH relayflowd --data-dir $DD resume $RID + exit=1 + Error: fold run journal + Caused by: + journal entry 5 appears after terminal run.completed +``` + +**Relaxing `ensure_mutable` to make the test green would let the daemon write +journals it then cannot resume.** That is a durability bug, and strictly worse +than a red test. The obvious "fix" is the wrong one, which is exactly why this +is escalated rather than patched. + +### 11.4 Two tests in the tree now assert opposite things + +The branch's own `crash_resume/protocol_admission.rs` asserts, for +`stream.append` and `event.emit` among six verbs, both the error code and that +the journal does not grow: + +```rust +for (verb, params) in cases { + assert_eq!(client.request_error_code(verb, params), "run_terminal"); + assert_eq!( + journal_entries(&fixture.data_dir).unwrap().len(), + before.len(), + "{verb} changed the terminal journal" + ); +} +``` + +Main's `live-kernel.test.ts` asserts the same two verbs **succeed** on a +terminal run. Both tests are in the post-rebase tree, and they cannot both pass. +This is not a merge artifact — each side is internally consistent and was +written deliberately. It is a genuine disagreement about the protocol that the +rebase surfaced. + +Note this rules out the tempting middle option: because `protocol_admission` +pins the **error code**, an accept-and-ignore compromise would require editing +the branch's own gate test too, not just main's. + +### 11.5 The decision, which is not the rebase owner's + +This is a protocol behaviour question with at least three shapes, and it is +**open**: + +1. **Keep the refusal, update the wire-conformance test.** Consistent with + `protocol_admission` and with the fold guard. But it removes a capability + main has, so it is a protocol change that belongs in the PR body as such, + not folded in as a test edit. +2. **Narrow the gate to accept-and-ignore** for these two verbs on a terminal + run — no journal write, returning `matched: 0` and the existing offset. + Preserves main's return contract *and* the fold guard, but it is behaviour + neither side has today, and it requires editing `protocol_admission` too + (§11.4), which is a gate judging this branch's own work. +3. **Drop the fold guard and the gate for these two verbs.** Restores main's + behaviour exactly; gives up the post-terminal journal invariant, and with it + the resume-safety shown in §11.3. + +I have not implemented any of them. Nothing in §11 changed a byte of the branch: + +``` +$ git status --short +(clean) +``` + +The throwaway worktree at `flows-main-512723c-wt` shares this worktree's +`node_modules` by symlink (no second install, no disk cost) and can be removed +with `git worktree remove`. From 4dec6af2fe94cb512184addaa22b0889d7d13941 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 14:48:14 +0200 Subject: [PATCH 12/14] test(sdk): a terminal run refuses event.emit and stream.append PROTOCOL CHANGE. This edits main's test to match a deliberate behaviour change in this branch, so it is its own commit, touching nothing else. The call was the lead's (relayflow-lead-0903), not this branch's. The signoff should re-derive it rather than inherit it. This is main's test -- "JournalClient wire conformance against live relayflowd". It started a deterministic run, which completes immediately, then called event.emit and stream.append on that terminal run and asserted both SUCCEED: expect((await client.eventEmit(run_id, 'unmatched', {ok:true})).matched).toBe(0); expect((await client.streamAppend(run_id, 'results', {answer:4})).offset).toBe(0); bdd598c admits every mutating verb through `ensure_mutable`, so this branch refuses both with run_terminal. Unlike the step.complete change in 269fcc6, this is not an error-code taxonomy move: observable behaviour on a shipped verb pair goes from accepted to refused. It does not remove a working capability. It removes a way to corrupt a journal that main reports as success. On main, stream.append against a terminal run journals stream.appended AFTER run.completed, and this branch's state.rs fold guard rejects exactly that -- so main produces journals the daemon cannot fold on resume. Same data directory, both binaries: MAIN resume -> exit=0 {"status":"completed","completion_reason":"success"} BRANCH resume -> exit=1 Error: fold run journal Caused by: journal entry 5 appears after terminal run.completed The realistic shape is worse than that synthetic one, and shows main is already self-inconsistent. A worker holds an llm lease; the run is cancelled out from under it; the worker then does what a live worker does: late step.complete -> THROW [lease_conflict] <- main already refuses this late stream.append -> OK {"offset":0} <- and corrupts the journal late event.emit -> OK {"matched":0} >>> entries AFTER terminal run.completed: ["stream.appended"] Main already holds "a terminal run accepts no step completions" (engine/remote.rs:44, from #142). This branch extends the same rule to the other mutating verbs, which is what the words already meant. Checked before changing anything, rather than assuming: no product code calls either verb (only the JournalClient method definitions); no doc sanctions a post-terminal append -- kernel/DESIGN.md:384-385 says event.emit "satisfies wait.event", which a terminal run has none of, and stream.append "journals stream.appended", which is the corruption; and the one in-repo live use (crash_resume/llm.rs) is against a parked run and is unaffected. The test keeps full wire coverage of both verbs. Their success paths move to the parked llm run, which is the only state in which appending to a run's journal is meaningful, and the terminal case now asserts the refusal -- matching crash_resume/protocol_admission.rs, which is left alone because it pins the invariant correctly. $ ./node_modules/.bin/tsc -p tsconfig.tests.json (clean) $ RELAYFLOWD_BIN= ./node_modules/.bin/vitest run Test Files 23 passed | 1 skipped (24) Tests 411 passed | 3 skipped (414) Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- sdk/tests/live-kernel.test.ts | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/sdk/tests/live-kernel.test.ts b/sdk/tests/live-kernel.test.ts index 431bbae38..333e73aa5 100644 --- a/sdk/tests/live-kernel.test.ts +++ b/sdk/tests/live-kernel.test.ts @@ -1206,11 +1206,20 @@ steps: expect((await replayed)['entry_type']).toBe('run.spawned'); const journal = await client.journalRead(deterministic.run_id, 1); expect(journal.entries.some((entry) => journalType(entry) === 'run.completed')).toBe(true); - expect((await client.eventEmit(deterministic.run_id, 'unmatched', { ok: true })).matched).toBe(0); - expect((await client.streamAppend(deterministic.run_id, 'results', { answer: 4 })).offset).toBe(0); + // `event.emit` and `stream.append` are mutations, and this run is terminal. + // Parallel dispatch (#137) admits every mutating verb through + // `ensure_mutable`, so both are refused here. They used to be accepted, and + // `stream.append` journalled `stream.appended` AFTER `run.completed` — + // producing a journal the daemon could no longer fold on resume. The + // success paths below exercise the same two verbs against a live run, which + // is the only state in which appending to a run's journal is meaningful. + await expect(client.eventEmit(deterministic.run_id, 'unmatched', { ok: true })) + .rejects.toMatchObject({ code: 'run_terminal' }); + await expect(client.streamAppend(deterministic.run_id, 'results', { answer: 4 })) + .rejects.toMatchObject({ code: 'run_terminal' }); expect(await client.streamRead(deterministic.run_id, 'results', 0, 10)).toEqual({ - messages: [{ answer: 4 }], - next_offset: 1, + messages: [], + next_offset: 0, }); const llmDispatch = eventOnce(client, 'step.dispatch'); @@ -1237,6 +1246,13 @@ steps: state: 'running', lease_deadline_ms: heartbeat.lease_deadline_ms, }); + // Wire coverage for the two mutating verbs, on a live run this time. + expect((await client.eventEmit(llmLease.run_id, 'unmatched', { ok: true })).matched).toBe(0); + expect((await client.streamAppend(llmLease.run_id, 'results', { answer: 4 })).offset).toBe(0); + expect(await client.streamRead(llmLease.run_id, 'results', 0, 10)).toEqual({ + messages: [{ answer: 4 }], + next_offset: 1, + }); const llmDone = await client.stepComplete( llmLease.run_id, llmLease.step_id, From 6b3ece00cccb637468cb0414bc95ca65a178feca Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 14:49:00 +0200 Subject: [PATCH 13/14] docs(reviews): record the option-1 resolution and the assumption test behind it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lead chose option 1 and asked that the assumption behind it be tested rather than inherited: is there any legitimate late stream.append or event.emit on a terminal run, out-of-band async completion in particular. Checked four ways and the assumption holds. No product code calls either verb — only the JournalClient definitions, a loopback mock, and tests. No doc sanctions a post-terminal append: DESIGN.md says event.emit satisfies a wait, which a terminal run has none of, and stream.append journals stream.appended, which is the corruption itself. The one live in-repo use appends against a parked run and is unaffected. The async shape was executed rather than reasoned about, and it cuts against main: a worker holding an llm lease on a run cancelled out from under it has its late step.complete refused with lease_conflict while its late stream.append is accepted in the same moment, journalling after run.completed and producing a journal the branch cannot fold. Main already holds the rule for step completions at engine/remote.rs:44; this branch extends it to the remaining mutating verbs. Adds the PR-body Protocol change section, and records a CI finding: the two contradicting tests both passed because neither runs in flows CI. cargo test never runs at all, and CI's vitest names four files, of which live-kernel.test.ts is not one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- ops/reviews/20260903-pr137-repair-0903.md | 163 +++++++++++++++++++++- 1 file changed, 159 insertions(+), 4 deletions(-) diff --git a/ops/reviews/20260903-pr137-repair-0903.md b/ops/reviews/20260903-pr137-repair-0903.md index 23f22c0d3..52fb4443b 100644 --- a/ops/reviews/20260903-pr137-repair-0903.md +++ b/ops/reviews/20260903-pr137-repair-0903.md @@ -1,4 +1,4 @@ -VERDICT: REBASED ONTO 512723c. ONE RED TEST, AND IT IS THE BRANCH'S — DIAGNOSIS IN §11, DECISION OPEN +VERDICT: REBASED ONTO 512723c, ALL GATES GREEN. CARRIES A PROTOCOL CHANGE — §11, §12. # PR #137 — rebase of `repair/pr137-uniform-reject-0903` onto `origin/main` (`512723c`) @@ -305,9 +305,10 @@ JournalProtocolError: run_terminal: run … is terminal and cannot accept mutati Test Files 1 failed | 22 passed | 1 skipped (24) Tests 1 failed | 410 passed | 3 skipped (414) EXIT=1 - 1 failure — the wire-conformance test. NOT pre-existing: it is - this branch's, and it is a behaviour change, not an error - code. Full diagnosis in §11. Push is on hold. + At the time of §11's diagnosis this run showed 1 failure, the + wire-conformance test. That was the branch's, not pre-existing, + and it was a behaviour change rather than an error code. Resolved + in 4dec6af (§12); the suite is now 411 passed | 3 skipped, 0 failed. ``` ### 6.1 Per-file accounting by test *name* @@ -686,3 +687,157 @@ $ git status --short The throwaway worktree at `flows-main-512723c-wt` shares this worktree's `node_modules` by symlink (no second install, no disk cost) and can be removed with `git worktree remove`. + +--- + +## 12. Resolution — option 1, and the assumption test that preceded it + +The lead chose **option 1: keep the refusal, update wire conformance, and +surface it as a protocol change** — and asked that the underlying assumption be +tested rather than inherited: *is there any legitimate reason to append to a +stream or emit an event after a run is terminal?* The named worry was out-of-band +step completion, since RFC-0001 supports a step an external worker finishes +asynchronously. + +### 12.1 The assumption holds — checked four ways + +**No product caller.** Every in-repo reference to these verbs, excluding reports: + +``` +kernel/relayflowd/src/server.rs:362,382 the handlers themselves +kernel/relayflowd/tests/crash_resume/llm.rs:32,38,56 live parked run +kernel/relayflowd/tests/crash_resume/protocol_admission.rs asserts the refusal +sdk/src/journal-client.ts:350,366 the method definitions +sdk/tests/journal-client.test.ts:191,227 loopback mock, no kernel +sdk/tests/live-kernel.test.ts:1209,1210 the test under discussion +``` + +Nothing in `sdk/src` beyond the two definitions, and nothing in `examples/`, +calls either verb. No product code depends on the accepted-when-terminal +behaviour. + +**No doc sanctions it.** `kernel/DESIGN.md:384-385` is the protocol contract: + +``` +| `event.emit` | {run_id, event_key, payload} → {matched: n} | satisfies `wait.event`; a human + response arrives here too, closing `wait.human` with completionReason: human_responded | +| `stream.append`| {run_id, stream, message} → {offset} | durable channel write; journals `stream.appended` | +``` + +`event.emit` exists to satisfy a **wait**, and a terminal run has none — which is +why main returns `matched: 0` there, doing nothing at all. `stream.append` +**journals `stream.appended`**, which on a terminal run is precisely the +corruption. Neither description contemplates a terminal target, and no file in +`docs/` or `kernel/DESIGN.md` mentions appending after completion. + +**The one live in-repo use is unaffected.** `crash_resume/llm.rs` appends and +emits against a run that is *parked* on an llm worker, not terminal. It passes +on this branch (part of `crash_resume` 34/34). + +**The async shape the lead worried about was executed, not reasoned about.** A +worker holds an llm lease; the run is cancelled out from under it; the worker +then does what a live worker does. On pure `512723c`: + +``` + run parked=parked; worker holds a lease on step answer + after run.cancel: status=failed reason=canceled (TERMINAL) + late step.complete -> THROW [lease_conflict] + late stream.append -> OK {"offset":0} + late event.emit -> OK {"matched":0} + journal: ["run.spawned","step.attempt.started","run.cancel.requested", + "step.completed","run.completed","stream.appended"] + >>> entries AFTER terminal run.completed: ["stream.appended"] +``` + +This is the strongest evidence in the whole diagnosis, and it cuts against main +rather than the branch. **Main already refuses the late `step.complete` from that +worker (`lease_conflict`) while accepting the late `stream.append` from the same +worker in the same moment** — and the append corrupts the journal: + +``` +$ MAIN relayflowd --data-dir $DD resume $RID + {"run_id":"01M1KMVQZ8T0MV3MDE1NCCHPEW","status":"failed","completion_reason":"canceled",...} +$ BRANCH relayflowd --data-dir $DD resume $RID + Error: fold run journal + Caused by: + journal entry 6 appears after terminal run.completed +``` + +Main is already internally inconsistent here. `engine/remote.rs:44` (from #142) +holds "a terminal run accepts no step completions"; this branch extends the same +rule to the remaining mutating verbs, which is what the words already meant. + +**Conclusion: no legitimate late append exists.** The assumption survives, and +option 2 (accept-and-ignore) would additionally have returned success for a write +that did not happen — a silent fallback, which AGENTS.md rule 4 forbids. + +### 12.2 What `4dec6af` changes + +Its own commit, `sdk/tests/live-kernel.test.ts` only, +20/−4. Both verbs keep +full wire coverage: their **success** paths move to the parked llm run — the only +state in which appending to a run's journal is meaningful — and the terminal case +now asserts the refusal, matching `protocol_admission`. `protocol_admission` is +left alone; it pinned the invariant correctly, and it is the other test that was +wrong. + +``` +$ ./node_modules/.bin/tsc -p tsconfig.tests.json +(clean) +$ RELAYFLOWD_BIN=…/411835410/debug/relayflowd ./node_modules/.bin/vitest run + Test Files 23 passed | 1 skipped (24) + Tests 411 passed | 3 skipped (414) +EXIT=0 +``` + +**All gates now green**: three `tsc` gates, `cargo test --workspace` 130/0, and +`vitest run` 411/0. + +### 12.3 For the PR body — Protocol change + +> **Protocol change: a terminal run refuses `event.emit` and `stream.append`.** +> +> Before this PR both were accepted on a run that had already completed. +> `stream.append` journalled `stream.appended` *after* `run.completed`, producing +> a journal the daemon could not fold on resume: +> +> ``` +> MAIN resume -> exit=0 {"status":"completed","completion_reason":"success"} +> BRANCH resume -> exit=1 Error: fold run journal +> Caused by: journal entry 5 appears after terminal run.completed +> ``` +> +> This does not remove a working capability; it removes a way to corrupt a +> journal that main currently reports as success. The wire-conformance test was +> asserting a bug. `main` already refused a late `step.complete` on a terminal run +> (`engine/remote.rs:44`, #142) while accepting a late `stream.append` from the +> same worker in the same moment; this makes the rule uniform. +> +> Callers that appended to a finished run now receive `run_terminal`. No in-repo +> product code did so. + +### 12.4 Why neither contradicting test caught this — a CI finding + +`protocol_admission` (Rust) and wire conformance (vitest) encoded **opposite +contracts and both passed**, because neither runs in flows CI: + +``` +$ ls .github/workflows/ +cloud-runtime-artifact.yml # the only workflow in the repo + +$ grep -rn "cargo test" .github/ +(none — CI runs `cargo build --locked --release -p relayflowd` only) + +$ sed -n '50,58p' .github/workflows/cloud-runtime-artifact.yml + ./node_modules/.bin/vitest run \ + tests/typed-output.test.ts \ + tests/validate.test.ts \ + tests/spec-parity.test.ts \ + tests/deterministic-llm.test.ts +``` + +The entire 130-test kernel suite never runs in CI, and CI's vitest invocation +names four files — **`live-kernel.test.ts` is not among them**. So neither side +of the contradiction was ever executed by CI, and the two could disagree +indefinitely while every PR stayed green. This is the same root cause behind the +lane's standing rule not to trust green flows CI, and it is worth a separate +issue rather than a footnote. From 7f6a30fd5bc19ee788e20d967d27fe67159debee Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 14:52:03 +0200 Subject: [PATCH 14/14] docs(reviews): lead the Protocol change section with main's self-inconsistency The strongest argument for the change is not the resume demonstration, it is that main already holds the rule and fails to apply it uniformly: a worker whose run was cancelled has its late step.complete refused with lease_conflict and its late stream.append accepted, in the same moment. This PR extends main's own rule rather than imposing a new one. Reordered so a reviewer meets that first, with the resume corruption as the reason it matters and DESIGN.md:384-385 as corroboration. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- ops/reviews/20260903-pr137-repair-0903.md | 38 +++++++++++++++++------ 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/ops/reviews/20260903-pr137-repair-0903.md b/ops/reviews/20260903-pr137-repair-0903.md index 52fb4443b..59bea4644 100644 --- a/ops/reviews/20260903-pr137-repair-0903.md +++ b/ops/reviews/20260903-pr137-repair-0903.md @@ -796,9 +796,25 @@ EXIT=0 > **Protocol change: a terminal run refuses `event.emit` and `stream.append`.** > -> Before this PR both were accepted on a run that had already completed. -> `stream.append` journalled `stream.appended` *after* `run.completed`, producing -> a journal the daemon could not fold on resume: +> **This is not a new rule. `main` already holds it, and simply fails to apply it +> consistently.** A worker holds an llm lease, the run is cancelled out from under +> it, and the worker does what a live worker does. On `main`, today: +> +> ``` +> late step.complete -> THROW [lease_conflict] <- main already refuses this +> late stream.append -> OK {"offset":0} <- and corrupts the journal +> late event.emit -> OK {"matched":0} +> >>> entries AFTER terminal run.completed: ["stream.appended"] +> ``` +> +> The late completion is refused and the late append is accepted, **from the same +> worker in the same moment**. `main` states the rule for step completions at +> `engine/remote.rs:44` (#142); this PR extends `main`'s own rule to the remaining +> mutating verbs. +> +> It matters because the accepted append corrupts the run. `stream.append` +> journals `stream.appended` *after* `run.completed`, producing a journal the +> daemon can no longer fold on resume — the same data directory, both binaries: > > ``` > MAIN resume -> exit=0 {"status":"completed","completion_reason":"success"} @@ -806,14 +822,18 @@ EXIT=0 > Caused by: journal entry 5 appears after terminal run.completed > ``` > -> This does not remove a working capability; it removes a way to corrupt a -> journal that main currently reports as success. The wire-conformance test was -> asserting a bug. `main` already refused a late `step.complete` on a terminal run -> (`engine/remote.rs:44`, #142) while accepting a late `stream.append` from the -> same worker in the same moment; this makes the rule uniform. +> So this does not remove a working capability; it removes a way to corrupt a +> journal that `main` currently reports as success. The wire-conformance test was +> asserting a bug. +> +> `kernel/DESIGN.md:384-385` agrees: `event.emit` "satisfies `wait.event`", which a +> terminal run has none of — so `main`'s `matched: 0` is a no-op wearing a success +> shape — and `stream.append` "journals `stream.appended`", which is the corruption +> itself. > > Callers that appended to a finished run now receive `run_terminal`. No in-repo -> product code did so. +> product code did so; the one live in-repo use appends against a *parked* run and +> is unaffected. ### 12.4 Why neither contradicting test caught this — a CI finding