diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index 72b0d78..5105f99 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -22,7 +22,9 @@ use crate::{ process::FakeProcess, shell::{MockShellTrait, ShellCommand, ShellOutput}, }, - kw::{actor::KwActor, history::MockKwHistoryStore}, + kw::{ + actor::KwActor, history::MockKwHistoryStore, messages::StartRequest, status::KwJobStatus, + }, lore::application::{ cache::BootstrapLoreData, handle::LoreApiHandle, messages::LoreApiMessage, }, @@ -251,6 +253,117 @@ async fn apply_failure_does_not_record_history() { shutdown_kw(&app).await; } +#[tokio::test] +async fn apply_is_blocked_while_a_kw_job_runs() { + // No scripted outputs: any apply-time git call fails the test. + let (shell, calls) = shell_with_outputs(vec![]); + let log_dir = kw_log_dir("apply-blocked"); + let mut app = app_with_details_and_kw( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config(), + history_store_allowing_writes(), + kw_actor_shell(), + kw_actor_fs(), + kw_actor_env(), + Arc::new(FakeProcess::new()), + log_dir.clone(), + ); + + let kw = app.services.kw.as_ref().expect("tests run on unix"); + kw.start_build(kw_start_request()).await.unwrap(); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_apply_action(&app, false); + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Blocked", + &["kw job is running", "Wait for the job to finish"], + ); + assert!(calls.lock().unwrap().is_empty()); + + shutdown_kw(&app).await; + std::fs::remove_dir_all(&log_dir).unwrap(); +} + +#[tokio::test] +async fn apply_is_allowed_again_after_the_job_finishes() { + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + let log_dir = kw_log_dir("apply-after-job"); + let process = Arc::new(FakeProcess::new()); + let mut store = MockKwHistoryStore::new(); + store.expect_record_apply().returning(|_| Ok(())); + store + .expect_apply_record_for_branch() + .returning(|_, _| Ok(None)); + store.expect_record_build().returning(|_| Ok(())); + let mut app = app_with_details_and_kw( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config(), + store, + kw_actor_shell(), + kw_actor_fs(), + kw_actor_env(), + process.clone(), + log_dir.clone(), + ); + + let kw = app.services.kw.as_ref().expect("tests run on unix").clone(); + kw.start_build(kw_start_request()).await.unwrap(); + + // While the job runs, the apply is blocked. + app.consolidate_patchset_actions().await.unwrap(); + assert_info_popup_contains(app.state.popup.as_ref(), "Patchset Apply Blocked", &[]); + assert!(calls.lock().unwrap().is_empty()); + + // Once the job finishes, the same apply goes through. + process.last_child().finish(0); + let mut watch = kw.watch_status().await.unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(10), async { + loop { + if matches!(watch.borrow().job, KwJobStatus::Succeeded { .. }) { + break; + } + watch.changed().await.unwrap(); + } + }) + .await + .expect("job must reach a terminal state"); + + let details = app + .state + .lore + .details + .as_mut() + .expect("details should remain loaded"); + details.toggle_apply_action(); + app.consolidate_patchset_actions().await.unwrap(); + + assert_apply_action(&app, false); + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Success", + &["applied successfully"], + ); + assert_eq!(6, calls.lock().unwrap().len()); + + shutdown_kw(&app).await; + std::fs::remove_dir_all(&log_dir).unwrap(); +} + #[tokio::test] async fn reviewed_reply_success_records_persists_and_resets_reply_action() { let saved_reviewed = Arc::new(Mutex::new(None)); @@ -351,16 +464,48 @@ fn app_with_details( details: PatchsetDetailsState, config: ConfigSnapshot, kw_history: MockKwHistoryStore, +) -> App { + app_with_details_and_kw( + fs, + shell, + lore_api, + details, + config, + kw_history, + MockShellTrait::new(), + MockFileSystemTrait::new(), + MockEnvTrait::new(), + Arc::new(FakeProcess::new()), + PathBuf::from("/tmp/patch-hub-test-kw-logs"), + ) +} + +/// Variant of [`app_with_details`] whose kw actor gets its own mocks, +/// process double, and log dir, for tests that start jobs through the +/// app's kw handle. +#[allow(clippy::too_many_arguments)] +fn app_with_details_and_kw( + fs: MockFileSystemTrait, + shell: MockShellTrait, + lore_api: LoreApiHandle, + details: PatchsetDetailsState, + config: ConfigSnapshot, + kw_history: MockKwHistoryStore, + kw_shell: MockShellTrait, + kw_fs: MockFileSystemTrait, + kw_env: MockEnvTrait, + kw_process: Arc, + kw_log_dir: PathBuf, ) -> App { // Apply history is recorded through the real actor wrapping the mock // store, mirroring production wiring. let kw = KwActor::spawn( Arc::new(kw_history), - Arc::new(FakeProcess::new()), - Arc::new(MockShellTrait::new()), - Arc::new(MockFileSystemTrait::new()), - Arc::new(MockEnvTrait::new()), - PathBuf::from("/tmp/patch-hub-test-kw-logs"), + kw_process, + Arc::new(kw_shell), + Arc::new(kw_fs), + Arc::new(kw_env), + kw_log_dir, ); let mut app = App::new( config, @@ -523,6 +668,80 @@ fn command_parts(cmd: &ShellCommand) -> Vec { parts } +/// kw-actor mocks for tests that start jobs through the app's handle: +/// kw on PATH at the verified version, clean git state, a ready kernel +/// tree — the happy path through the actor's start probes. +fn kw_actor_shell() -> MockShellTrait { + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(|cmd| { + let stdout = match cmd.program.as_str() { + "kw" => b"kw, version 0.10.0\n".to_vec(), + _ if cmd.args.iter().any(|arg| arg == "status") => Vec::new(), + _ => b"main\n".to_vec(), + }; + Ok(ShellOutput { + stdout, + stderr: Vec::new(), + success: true, + }) + }); + shell +} + +fn kw_actor_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(|_| { + Err(FileSystemError::IoError(std::io::Error::new( + std::io::ErrorKind::NotFound, + "missing", + ))) + }); + fs.expect_read_dir().returning(|_| { + Err(FileSystemError::IoError(std::io::Error::new( + std::io::ErrorKind::NotFound, + "missing", + ))) + }); + fs.expect_create_dir_all().returning(|_| Ok(())); + fs +} + +fn kw_actor_env() -> MockEnvTrait { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env +} + +/// A real, unique directory: FakeProcess creates the job's log file on +/// spawn, even though the fs trait is mocked. +fn kw_log_dir(test_name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "patch-hub-app-kw-logs-{}-{}", + test_name, + std::process::id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +fn kw_start_request() -> StartRequest { + StartRequest { + kernel_tree_id: "linux".to_string(), + tree: serde_json::from_value(serde_json::json!({ + "path": KERNEL_TREE_PATH, + "branch": BASE_BRANCH + })) + .expect("kernel tree should deserialize"), + branch: "patchset-2026-08-20-15-00-00".to_string(), + extra_args: Vec::new(), + } +} + fn command(parts: &[&str]) -> Vec { parts.iter().map(|part| part.to_string()).collect() } diff --git a/src/app/mod.rs b/src/app/mod.rs index c10819c..7d1a0bf 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -50,6 +50,7 @@ use crate::{ kw::{ handle::KwHandle, history::{KwApplyRecord, KwHistoryStore}, + status::KwJobStatus, }, lore::{ application::{ @@ -427,56 +428,12 @@ impl App { if patchset_action_selected(details, &PatchsetAction::Apply) { debug!("applying patchset via git-am"); - let request = apply_patchset_request(details); - let action_service = PatchsetActionService::new( - &*self.services.fs, - &*self.services.shell, - &self.services.lore_api, - ); - let popup = match action_service.apply_patchset(&request, &self.state.config) { - Ok(applied) => { - let popup_body = match kw_apply_record(details, &self.state.config, &applied) { - // Defensive: the apply itself resolved this tree from - // the same snapshot, so this is unreachable unless the - // config changed mid-apply. - None => { - warn!("kw apply history skipped: target kernel tree is no longer configured"); - format!( - "{}\n\nWarning: the apply was not recorded in the kw history: target kernel tree is no longer configured", - applied.message - ) - } - Some(record) => { - // History writes go through KwActor so apply - // recording serializes with job state; without an - // actor (non-unix), write the store directly. - let recorded = match &self.services.kw { - Some(kw) => { - kw.record_apply(record).await.map_err(|e| e.to_string()) - } - None => self - .services - .kw_history - .record_apply(record) - .map_err(|e| e.to_string()), - }; - // The git apply itself succeeded; a history-write - // failure must not turn it into a reported failure. - match recorded { - Ok(()) => applied.message, - Err(e) => { - warn!(error = %e, "failed to record kw apply history"); - format!( - "{}\n\nWarning: the apply was not recorded in the kw history: {e}\nIf this warning keeps appearing, inspect or delete that file.", - applied.message - ) - } - } - } - }; - popup::AppPopup::info("Patchset Apply Success", popup_body) - } - Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), + // A running kw job owns the tree; applying would rewrite the + // branch it is building. AppActor serializes this with Start, + // so the two cannot race. + let popup = match self.kw_job_running_popup().await { + Some(popup) => popup, + None => self.apply_patchset_popup(details).await, }; self.state.popup = Some(popup); @@ -490,6 +447,78 @@ impl App { } } + /// The popup blocking an apply while a kw job runs, if a job is in + /// fact running. An unreachable actor cannot be running a job, so a + /// status-query failure lets the apply proceed. + async fn kw_job_running_popup(&self) -> Option { + let kw = self.services.kw.as_ref()?; + match kw.get_status().await { + Ok(snapshot) if matches!(snapshot.job, KwJobStatus::Running { .. }) => { + Some(popup::AppPopup::info( + "Patchset Apply Blocked", + " A kw job is running on the kernel tree.\n\nApplying a patchset now would rewrite the branch the job is building under it.\n\nWait for the job to finish, then apply again.", + )) + } + _ => None, + } + } + + /// Runs the git-am apply and maps the outcome to the result popup, + /// recording the apply in the kw history on success. + async fn apply_patchset_popup(&self, details: &PatchsetDetailsState) -> popup::AppPopup { + let request = apply_patchset_request(details); + let action_service = PatchsetActionService::new( + &*self.services.fs, + &*self.services.shell, + &self.services.lore_api, + ); + match action_service.apply_patchset(&request, &self.state.config) { + Ok(applied) => { + let popup_body = match kw_apply_record(details, &self.state.config, &applied) { + // Defensive: the apply itself resolved this tree from + // the same snapshot, so this is unreachable unless the + // config changed mid-apply. + None => { + warn!( + "kw apply history skipped: target kernel tree is no longer configured" + ); + format!( + "{}\n\nWarning: the apply was not recorded in the kw history: target kernel tree is no longer configured", + applied.message + ) + } + Some(record) => { + // History writes go through KwActor so apply + // recording serializes with job state; without an + // actor (non-unix), write the store directly. + let recorded = match &self.services.kw { + Some(kw) => kw.record_apply(record).await.map_err(|e| e.to_string()), + None => self + .services + .kw_history + .record_apply(record) + .map_err(|e| e.to_string()), + }; + // The git apply itself succeeded; a history-write + // failure must not turn it into a reported failure. + match recorded { + Ok(()) => applied.message, + Err(e) => { + warn!(error = %e, "failed to record kw apply history"); + format!( + "{}\n\nWarning: the apply was not recorded in the kw history: {e}\nIf this warning keeps appearing, inspect or delete that file.", + applied.message + ) + } + } + } + }; + popup::AppPopup::info("Patchset Apply Success", popup_body) + } + Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), + } + } + /// Opens the edit-config screen from the current configuration snapshot. pub fn init_edit_config(&mut self) { self.state.config_state.edit_config = Some(EditConfigState::new(&self.state.config)); diff --git a/src/infrastructure/process/fake.rs b/src/infrastructure/process/fake.rs index 1cb84eb..aab609b 100644 --- a/src/infrastructure/process/fake.rs +++ b/src/infrastructure/process/fake.rs @@ -46,6 +46,10 @@ struct FakeState { /// "running": the model for a process group that ignores SIGTERM, so /// tests can exercise the SIGKILL escalation. ignores_sigterm: bool, + /// When set, `wait()` resolves with an IO error once the process + /// finishes: the model for a process whose exit status is lost (an + /// already-reaped child, an OS-level wait failure). + fails_wait: bool, } impl FakeControl { @@ -90,6 +94,7 @@ pub struct FakeProcess { spawns: Mutex>, refuse_spawns: AtomicBool, ignore_sigterm: AtomicBool, + fail_waits: AtomicBool, } impl FakeProcess { @@ -110,6 +115,12 @@ impl FakeProcess { self.ignore_sigterm.store(ignore, Ordering::Relaxed); } + /// Make subsequently spawned processes' `wait()` resolve with an IO + /// error once they finish, instead of their exit status. + pub fn fail_waits(&self, fail: bool) { + self.fail_waits.store(fail, Ordering::Relaxed); + } + pub fn spawned(&self) -> Vec { self.spawns .lock() @@ -153,6 +164,7 @@ impl ProcessTrait for FakeProcess { killed: false, force_killed: false, ignores_sigterm: self.ignore_sigterm.load(Ordering::Relaxed), + fails_wait: self.fail_waits.load(Ordering::Relaxed), }), notify: Notify::new(), log_path: log_path.to_path_buf(), @@ -179,11 +191,20 @@ struct FakeRunningProcess { impl RunningProcess for FakeRunningProcess { async fn wait(&mut self) -> Result { loop { - // The state lock is released before the await; a finish()/kill() - // racing the check is not lost because Notify stores one permit. - let raw_status = self.control.state.lock().unwrap().raw_status; - if let Some(raw) = raw_status { - return Ok(ExitStatus::from_raw(raw)); + // Only Copy data escapes the guard scope, so the (non-Send) + // guard is never held across the await; a finish()/kill() + // racing the check is not lost because Notify stores one + // permit. + let finished = { + let state = self.control.state.lock().unwrap(); + state.raw_status.map(|raw| (raw, state.fails_wait)) + }; + if let Some((raw, fails_wait)) = finished { + return if fails_wait { + Err(ProcessError::IoError(io::Error::other("fake wait failure"))) + } else { + Ok(ExitStatus::from_raw(raw)) + }; } self.control.notify.notified().await; } diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 3907f70..3a82131 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -8,10 +8,16 @@ //! [`crate::kw::readiness`] and records applies through the shared //! [`KwHistoryStore`](crate::kw::history::KwHistoryStore). //! -//! `create_dir_all`, the HEAD-probe `git` call, and history writes run -//! inline on the actor task. - -use std::{ops::ControlFlow, path::PathBuf, process::ExitStatus, sync::Arc, time::Duration}; +//! Git checkout runs on the blocking pool. `create_dir_all`, history +//! writes, and completion probes run inline on the actor task. + +use std::{ + ops::ControlFlow, + path::{Path, PathBuf}, + process::ExitStatus, + sync::Arc, + time::Duration, +}; use tokio::{ spawn, @@ -27,11 +33,12 @@ use crate::{ shell::{ShellCommand, ShellTrait}, }, kw::{ - errors::{KwError, KwStartError}, + argv, + errors::{KwError, KwStartError, TreeGitError}, handle::KwHandle, - history::KwHistoryStore, + history::{KwBuildRecord, KwHistoryStore}, messages::{KwMessage, StartRequest}, - readiness::{self, KwReadiness}, + readiness::{self, KwReadiness, KwVersionCheck, TreeReadiness}, status::{KwJobKind, KwJobStatus, KwPhase, KwStatusSnapshot}, }, }; @@ -60,6 +67,14 @@ enum JobEvent { Finished(JobOutcome), } +/// Where RestorePreviousBranch switches back to: the branch HEAD was on +/// when the last job was accepted, and the tree that branch lives in. +/// Session-only, deliberately not persisted. +struct RestoreContext { + tree_path: String, + branch: String, +} + /// What the actor remembers about the running job while the detached task /// owns the process itself (see [`run_job`]). struct JobState { @@ -67,11 +82,14 @@ struct JobState { phase: KwPhase, kernel_tree_id: String, branch: String, + /// Tree path, kw-env output dir, and build arch as probed at accept + /// time. The build runs under these, so the completion record + /// describes this snapshot — not whatever the tree's configuration + /// says by the time the job ends. + tree_path: String, + output_dir: Option, + arch: Option, log_path: PathBuf, - /// HEAD at accept time, so RestorePreviousBranch can offer to switch - /// back after the job. Read once the checkout policy lands. - #[allow(dead_code)] - pre_job_branch: Option, /// `None` once a cancel has been requested; a second `Cancel` is an /// idempotent ack. cancel_tx: Option>, @@ -89,6 +107,9 @@ pub struct KwActor { process: Arc, kw_log_dir: PathBuf, job: Option, + /// Recorded only when a job is accepted: a refused start never + /// clobbers a previous job's restore target. + last_restore: Option, } impl KwActor { @@ -115,6 +136,7 @@ impl KwActor { process, kw_log_dir, job: None, + last_restore: None, } } @@ -169,7 +191,7 @@ impl KwActor { send_start_reply( message_name, reply, - self.start_job(KwJobKind::Build, request), + self.start_job(KwJobKind::Build, request).await, ); ControlFlow::Continue(()) } @@ -203,10 +225,8 @@ impl KwActor { ); ControlFlow::Continue(()) } - // Restoring needs the dirty-worktree check and `git switch`, - // which arrive with the checkout policy in the build step. KwMessage::RestorePreviousBranch { reply } => { - send_kw_reply(message_name, reply, Err(KwError::NoRecordedBranch)); + send_kw_reply(message_name, reply, self.restore_previous_branch().await); ControlFlow::Continue(()) } KwMessage::Shutdown { reply } => { @@ -241,30 +261,58 @@ impl KwActor { /// caller right after this returns: the job itself keeps running in a /// detached task and is observed via the status snapshot. /// - /// The argv is `kw build --alert=n`. The `branch` on the Running status - /// is the requested branch; this skeleton does not switch the tree. - fn start_job(&mut self, kind: KwJobKind, request: StartRequest) -> Result<(), KwStartError> { + /// Refusals, in order: a job already running, no kw binary on PATH, + /// unresolvable kw-env state, a tree that fails the readiness probes, + /// a dirty worktree, or a failed branch switch. Reserved flags on + /// patch-hub's own argv win over the request's extra args. A start + /// refused after the branch switch rolls the switch back: a refused + /// start never leaves the tree on a branch the user did not check out. + async fn start_job( + &mut self, + kind: KwJobKind, + request: StartRequest, + ) -> Result<(), KwStartError> { if self.job.is_some() { return Err(KwStartError::JobAlreadyRunning); } - // Recorded at accept so RestorePreviousBranch can switch back. - // An unprobed HEAD (detached, or not a git repo) records nothing. - let pre_job_branch = match self.head_branch(&request.tree) { - branch if branch.is_empty() => None, - branch => Some(branch), + // Hard fail on invoke: with no kw binary on PATH no job can run. + // The version check is advisory only — kw's shipped VERSION file + // is stale, so Below/Unknown are logged, never gated. + let kw_binary = readiness::probe_kw_binary(&*self.env, &*self.shell); + if !kw_binary.available { + return Err(KwStartError::KwBinaryMissing); + } + if let KwVersionCheck::Below(version_line) = &kw_binary.check { + tracing::warn!( + version = %version_line, + minimum = ?readiness::KW_MIN_VERSION, + "kw reports a version below the verified floor" + ); + } + + let tree_path = PathBuf::from(request.tree.path()); + // Unresolvable env state refuses the start: the build record this + // job writes at completion must know whether it ran under an O=. + // Both values are then snapshotted onto the job — the build runs + // under them, so the completion record describes them, not the + // tree's configuration at whatever time the job ends. + let output_dir = readiness::resolve_output_dir(&*self.fs, &*self.env, &tree_path)?; + let tree_readiness = readiness::probe_tree(&*self.fs, &tree_path, output_dir.as_deref()); + let TreeReadiness::Ready { arch } = tree_readiness else { + return Err(KwStartError::TreeNotReady(tree_readiness)); }; - self.fs.create_dir_all(&self.kw_log_dir)?; - // Millisecond suffix: two jobs started within the same second must - // not share a log file — spawn truncates it. - let log_path = self.kw_log_dir.join(format!( - "build-{}.log", - chrono::Utc::now().format("%Y%m%d-%H%M%S-%3f") - )); - let cmd = ShellCommand::new("kw").args(["build", "--alert=n"]); - let cwd = PathBuf::from(request.tree.path()); - let process = self.process.spawn(&cmd, &cwd, &log_path)?; + let pre_job_branch = self.checkout_build_branch(&request).await?; + + let (process, log_path) = match self.spawn_build_process(&request) { + Ok(spawned) => spawned, + Err(error) => { + self.rollback_switch(request.tree.path(), pre_job_branch.as_deref()) + .await; + return Err(error); + } + }; let (cancel_tx, cancel_rx) = oneshot::channel(); spawn(run_job(process, cancel_rx, self.job_event_tx.clone())); @@ -276,13 +324,24 @@ impl KwActor { log_path = %log_path.display(), "kw build job started" ); + // Recorded only on accept. Refused starts never reach here, and — + // with their switch rolled back — can neither clobber a previous + // job's restore target nor strand the tree on a branch the user + // did not check out. An accepted job with an unprobed pre-job + // HEAD clears the target: nothing honest is left to restore to. + self.last_restore = pre_job_branch.map(|branch| RestoreContext { + tree_path: request.tree.path().to_string(), + branch, + }); self.job = Some(JobState { kind, phase, kernel_tree_id: request.kernel_tree_id.clone(), branch: request.branch.clone(), + tree_path: request.tree.path().to_string(), + output_dir, + arch, log_path: log_path.clone(), - pre_job_branch, cancel_tx: Some(cancel_tx), }); self.set_status(KwJobStatus::Running { @@ -295,6 +354,94 @@ impl KwActor { Ok(()) } + /// Refuse a dirty worktree, record HEAD, then `git switch` to the + /// requested branch. The git calls run on the blocking pool so the + /// accept reply stays immediate. + /// + /// The HEAD probe sits between the dirty check and the switch: it + /// must capture the branch the user was on, or RestorePreviousBranch + /// would restore the branch the job switched to. An unprobed HEAD + /// (detached, or not a git repo) yields `None` rather than a wrong + /// branch. + async fn checkout_build_branch( + &self, + request: &StartRequest, + ) -> Result, KwStartError> { + let shell = Arc::clone(&self.shell); + let tree_path = request.tree.path().to_string(); + let branch = request.branch.clone(); + let pre_job_branch = + tokio::task::spawn_blocking(move || -> Result { + check_worktree_clean(&*shell, &tree_path)?; + let pre_job_branch = head_branch(&*shell, &tree_path); + switch_to_branch(&*shell, &tree_path, &branch)?; + Ok(pre_job_branch) + }) + .await + // A join error means the probe task panicked — a bug, surfaced as + // an unverifiable git state rather than wedging the actor. + .map_err(|error| KwStartError::GitStateProbe(error.to_string()))??; + Ok(if pre_job_branch.is_empty() { + None + } else { + Some(pre_job_branch) + }) + } + + /// Switches the tree back after a start that reached the branch + /// switch but failed before accepting the job. A rollback failure is + /// logged, not reported: the caller already holds the actionable + /// refusal. An unprobed pre-job HEAD leaves nothing to roll back to. + async fn rollback_switch(&self, tree_path: &str, pre_job_branch: Option<&str>) { + let Some(branch) = pre_job_branch else { + tracing::warn!( + tree = tree_path, + "cannot roll back the branch switch: pre-job HEAD was unprobed" + ); + return; + }; + let shell = Arc::clone(&self.shell); + let tree_path = tree_path.to_string(); + let branch = branch.to_string(); + let branch_for_task = branch.clone(); + match tokio::task::spawn_blocking(move || { + switch_to_branch(&*shell, &tree_path, &branch_for_task) + }) + .await + { + Ok(Ok(())) => tracing::info!( + branch, + "rolled back the branch switch after a refused start" + ), + Ok(Err(error)) => { + let error = KwStartError::from(error); + tracing::warn!(%error, "failed to roll back the branch switch") + } + Err(error) => { + tracing::warn!(%error, "branch-switch rollback task failed to join") + } + } + } + + /// Creates the job's log dir and spawns the kw process. Split from + /// `start_job` so a failure here can roll the branch switch back. + fn spawn_build_process( + &self, + request: &StartRequest, + ) -> Result<(Box, PathBuf), KwStartError> { + self.fs.create_dir_all(&self.kw_log_dir)?; + // Millisecond suffix: two jobs started within the same second must + // not share a log file — spawn truncates it. + let log_path = self.kw_log_dir.join(format!( + "build-{}.log", + chrono::Utc::now().format("%Y%m%d-%H%M%S-%3f") + )); + let cmd = ShellCommand::new("kw").args(argv::build_argv(&request.extra_args)); + let cwd = PathBuf::from(request.tree.path()); + let process = self.process.spawn(&cmd, &cwd, &log_path)?; + Ok((process, log_path)) + } + fn request_cancel(&mut self) -> Result<(), KwError> { match self.job.as_mut() { Some(job) => { @@ -314,6 +461,10 @@ impl KwActor { tracing::warn!("kw job finished with no job state recorded"); return; }; + // The record is written before the status flips: watchers + // that react to the terminal status find the history + // already durable. + self.record_build_outcome(&job, &outcome); let status = match outcome { JobOutcome::Exited(exit) if exit.success() => { tracing::info!( @@ -364,6 +515,74 @@ impl KwActor { } } + /// Persist a finished build (success or failure). Cancel writes + /// nothing. History and patchset-link errors are logged, not folded + /// into job status — the build's real outcome already reached the user. + fn record_build_outcome(&self, job: &JobState, outcome: &JobOutcome) { + let success = match outcome { + JobOutcome::Exited(exit) => exit.success(), + // The exit status is lost: record an honest failure rather + // than guess, so deploy-alone readiness cannot trust it. + JobOutcome::WaitFailed(_) => false, + JobOutcome::Cancelled => return, + }; + + // The record describes the accept-time snapshot: the build ran + // under this tree path, output dir, and arch. Re-resolving them + // here could describe a configuration the build never used — an + // env deactivated mid-build would key the record with a wrong + // `output_dir: None` and probe the tree for an image the build + // wrote under O=, a Frankenstein match for a later deploy-alone + // probe. + let tree_path = Path::new(&job.tree_path); + let build_root = job.output_dir.as_deref().unwrap_or(tree_path); + // A failed build may have left a stale image from an earlier + // successful one behind; only successes record what they + // produced. + let (image_path, kernelrelease) = if success { + ( + readiness::find_newest_kernel_image(&*self.fs, build_root, job.arch.as_deref()), + readiness::read_kernelrelease(&*self.fs, build_root), + ) + } else { + (None, None) + }; + let message_id = match self + .history + .apply_record_for_branch(&job.kernel_tree_id, &job.branch) + { + Ok(record) => record.map(|record| record.message_id), + Err(error) => { + tracing::warn!( + %error, + branch = job.branch, + "build record loses its patchset link" + ); + None + } + }; + + let record = KwBuildRecord { + kernel_tree_id: job.kernel_tree_id.clone(), + tree_path: job.tree_path.clone(), + message_id, + branch: job.branch.clone(), + arch: job.arch.clone(), + image_path: image_path.map(|path| path.to_string_lossy().into_owned()), + output_dir: job + .output_dir + .as_ref() + .map(|path| path.to_string_lossy().into_owned()), + kernelrelease, + log_path: job.log_path.to_string_lossy().into_owned(), + built_at: chrono::Utc::now().to_rfc3339(), + success, + }; + if let Err(error) = self.history.record_build(record) { + tracing::warn!(%error, branch = job.branch, "failed to record kw build history"); + } + } + fn set_status(&mut self, job: KwJobStatus) { // send_replace, not send: no receiver (nobody called WatchStatus // yet) is a normal state, not an error. @@ -375,7 +594,7 @@ impl KwActor { kernel_tree_id: &str, tree: &KernelTree, ) -> Result { - let head = self.head_branch(tree); + let head = head_branch(&*self.shell, tree.path()); Ok(readiness::evaluate_readiness( &*self.fs, &*self.env, @@ -387,33 +606,118 @@ impl KwActor { )?) } - /// The tree's current branch, probed via git. An unresolvable HEAD - /// (not a git repo, or a detached HEAD, for which `branch - /// --show-current` prints nothing) yields an empty string: no build - /// record can match it, so deploy-alone readiness refuses — the safe - /// direction for an unknown HEAD. - fn head_branch(&self, tree: &KernelTree) -> String { - let cmd = ShellCommand::new("git").args(["-C", tree.path(), "branch", "--show-current"]); - match self.shell.execute(&cmd) { - Ok(output) if output.success => { - String::from_utf8_lossy(&output.stdout).trim().to_string() - } - Ok(output) => { - tracing::warn!( - tree = tree.path(), - stderr = %String::from_utf8_lossy(&output.stderr), - "failed to probe the kernel tree's HEAD branch" + /// Switches the tree that ran the last job back to the branch HEAD was + /// on when that job was accepted. Refuses while a job is + /// running (its branch is in use), when nothing was recorded, and on + /// a dirty worktree. Only a successful switch consumes the context — + /// a refused restore stays available for a retry. + async fn restore_previous_branch(&mut self) -> Result<(), KwError> { + if self.job.is_some() { + return Err(KwError::JobRunning); + } + let Some(restore) = self.last_restore.take() else { + return Err(KwError::NoRecordedBranch); + }; + let shell = Arc::clone(&self.shell); + let tree_path = restore.tree_path.clone(); + let branch = restore.branch.clone(); + // Same spawn_blocking rationale as the start path's checkout: the + // switch rewrites the worktree. + let result = tokio::task::spawn_blocking(move || -> Result<(), TreeGitError> { + check_worktree_clean(&*shell, &tree_path)?; + switch_to_branch(&*shell, &tree_path, &branch) + }) + .await + .map_err(|error| KwError::GitStateProbe(error.to_string())) + .and_then(|result| result.map_err(KwError::from)); + match result { + Ok(()) => { + tracing::info!( + tree = restore.tree_path, + branch = restore.branch, + "restored pre-job branch" ); - String::new() + Ok(()) } Err(error) => { - tracing::warn!(tree = tree.path(), %error, "failed to probe the kernel tree's HEAD branch"); - String::new() + self.last_restore = Some(restore); + Err(error) } } } } +/// Fails unless the tree's git state verifies clean. Untracked files +/// don't count: kernel trees accumulate local scratch files, and only +/// tracked changes can corrupt the branch a job builds. A probe that +/// itself fails — git missing, not a repository — fails too. +fn check_worktree_clean(shell: &dyn ShellTrait, tree_path: &str) -> Result<(), TreeGitError> { + let cmd = ShellCommand::new("git").args([ + "-C", + tree_path, + "status", + "--porcelain", + "--untracked-files=no", + ]); + let output = shell + .execute(&cmd) + .map_err(|error| TreeGitError::Probe(error.to_string()))?; + if !output.success { + return Err(TreeGitError::Probe( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )); + } + if !output.stdout.is_empty() { + return Err(TreeGitError::DirtyWorktree); + } + Ok(()) +} + +/// Switches the tree onto `branch`. A failure carries git's stderr, which +/// names the usual causes (no such branch, a rebase or merge in +/// progress). The `--` keeps a branch named like a flag from being parsed +/// as one. +fn switch_to_branch( + shell: &dyn ShellTrait, + tree_path: &str, + branch: &str, +) -> Result<(), TreeGitError> { + let cmd = ShellCommand::new("git").args(["-C", tree_path, "switch", "--", branch]); + let output = shell + .execute(&cmd) + .map_err(|error| TreeGitError::Switch(error.to_string()))?; + if !output.success { + return Err(TreeGitError::Switch( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )); + } + Ok(()) +} + +/// The tree's current branch, probed via git. An unresolvable HEAD +/// (not a git repo, or a detached HEAD, for which `branch +/// --show-current` prints nothing) yields an empty string: no build +/// record can match it, so deploy-alone readiness refuses — the safe +/// direction for an unknown HEAD. +fn head_branch(shell: &dyn ShellTrait, tree_path: &str) -> String { + let cmd = ShellCommand::new("git").args(["-C", tree_path, "branch", "--show-current"]); + match shell.execute(&cmd) { + Ok(output) if output.success => String::from_utf8_lossy(&output.stdout).trim().to_string(), + Ok(output) => { + tracing::warn!( + tree = tree_path, + stderr = %String::from_utf8_lossy(&output.stderr), + "failed to probe the kernel tree's HEAD branch" + ); + String::new() + } + Err(error) => { + tracing::warn!(tree = tree_path, %error, "failed to probe the kernel tree's HEAD branch"); + String::new() + } + } +} + /// Owns the spawned process until it ends: waits on it, or — when the /// cancel signal fires — runs the kill escalation and reaps it. The /// `wait()` future is dropped before the cancel arm's body runs, releasing @@ -480,10 +784,18 @@ fn outcome_after_cancel(result: Result) -> JobOutcome use std::os::unix::process::ExitStatusExt; match result { - Ok(status) => match status.signal() { - Some(_) => JobOutcome::Cancelled, - None => JobOutcome::Exited(status), - }, + Ok(status) => { + if status.signal().is_some() { + return JobOutcome::Cancelled; + } + // kw is a bash wrapper: a process-group SIGTERM/SIGKILL often + // surfaces as a plain exit of 128+signum (143 / 137) rather than + // WIFSIGNALED. After a cancel request those are still cancels. + match status.code() { + Some(143 | 137) => JobOutcome::Cancelled, + _ => JobOutcome::Exited(status), + } + } Err(error) => JobOutcome::WaitFailed(error), } } @@ -544,7 +856,10 @@ mod tests { use std::{ io, path::Path, - sync::atomic::{AtomicU64, Ordering}, + sync::{ + atomic::{AtomicBool, AtomicU64, Ordering}, + Mutex, + }, time::Duration, }; @@ -553,11 +868,11 @@ mod tests { env::MockEnvTrait, file_system::{FileSystemError, MockFileSystemTrait}, process::FakeProcess, - shell::{MockShellTrait, ShellOutput}, + shell::{MockShellTrait, ShellCommand, ShellOutput}, }, kw::{ errors::KwStartError, - history::{KwApplyRecord, MockKwHistoryStore}, + history::{KwApplyRecord, KwBuildRecord, MockKwHistoryStore}, messages::StartRequest, readiness::{DeployAloneRefusal, TreeReadiness}, status::{KwJobKind, KwJobStatus, KwPhase}, @@ -594,6 +909,7 @@ mod tests { kernel_tree_id: "mainline".to_string(), tree: kernel_tree(Path::new("/home/user/linux")), branch: "patchset-2026-08-01-17-30-00".to_string(), + extra_args: Vec::new(), } } @@ -619,38 +935,272 @@ mod tests { ) } - /// Spawns the actor with a real temp log dir and exposes the - /// [`FakeProcess`] so tests drive the "running" process. The shell mock - /// answers the pre-job HEAD probe. - fn spawn_job_actor_with_fs( + const KW_VERSION_OK: &[u8] = b"kw, version 0.10.0\n"; + /// A clean `git status --porcelain` answer. + const CLEAN_STATUS: (&[u8], &[u8], bool) = (b"", b"", true); + /// A successful `git switch` answer. + const SWITCH_OK: (&[u8], bool) = (b"", true); + + fn command_parts(cmd: &ShellCommand) -> Vec { + let mut parts = vec![cmd.program.clone()]; + parts.extend(cmd.args.clone()); + parts + } + + fn command(parts: &[&str]) -> Vec { + parts.iter().map(|part| part.to_string()).collect() + } + + /// A shell mock that logs every command's argv parts and answers by + /// content: kw's version probe with `kw_version`; `git status + /// --porcelain` with `status` (stdout, stderr, success); `git switch` + /// with `switch` (stderr, success); any other git call — the HEAD + /// branch probe — with `master`. + fn recording_shell( + kw_version: &'static [u8], + status: (&'static [u8], &'static [u8], bool), + switch: (&'static [u8], bool), + ) -> (MockShellTrait, Arc>>>) { + let calls = Arc::new(Mutex::new(Vec::new())); + let calls_in_shell = Arc::clone(&calls); + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(move |cmd| { + calls_in_shell.lock().unwrap().push(command_parts(cmd)); + let output = |stdout: &[u8], stderr: &[u8], success: bool| ShellOutput { + stdout: stdout.to_vec(), + stderr: stderr.to_vec(), + success, + }; + if cmd.program == "kw" { + return Ok(output(kw_version, b"", true)); + } + if cmd.args.iter().any(|arg| arg == "status") { + return Ok(output(status.0, status.1, status.2)); + } + if cmd.args.iter().any(|arg| arg == "switch") { + return Ok(output(b"", switch.0, switch.1)); + } + Ok(output(b"master\n", b"", true)) + }); + (shell, calls) + } + + /// A stateful shell double for the checkout/restore tests: kw's + /// version probe answers 0.10.0, `git status --porcelain` reflects the + /// dirty flag, the HEAD probe reports `head` (with the trailing + /// newline git prints), and a successful `git switch` updates `head` + /// — mirroring a real worktree the actor switches between branches. + /// Switches to `fail_switch_to` fail, so a test can model "going + /// forward works, coming back fails". + struct GitStub { + head: Arc>, + dirty: Arc, + fail_switch_to: Arc>>, + } + + impl GitStub { + fn on_branch(branch: &str) -> Self { + Self { + head: Arc::new(Mutex::new(branch.to_string())), + dirty: Arc::new(AtomicBool::new(false)), + fail_switch_to: Arc::new(Mutex::new(None)), + } + } + + fn set_dirty(&self, dirty: bool) { + self.dirty.store(dirty, Ordering::Relaxed); + } + + fn fail_switches_to(&self, branch: Option<&str>) { + *self.fail_switch_to.lock().unwrap() = branch.map(str::to_string); + } + + fn head(&self) -> String { + self.head.lock().unwrap().clone() + } + + fn shell(&self) -> MockShellTrait { + let head = Arc::clone(&self.head); + let dirty = Arc::clone(&self.dirty); + let fail_switch_to = Arc::clone(&self.fail_switch_to); + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(move |cmd| { + let output = |stdout: &[u8]| ShellOutput { + stdout: stdout.to_vec(), + stderr: Vec::new(), + success: true, + }; + if cmd.program == "kw" { + return Ok(output(KW_VERSION_OK)); + } + if cmd.args.iter().any(|arg| arg == "status") { + let stdout: &[u8] = if dirty.load(Ordering::Relaxed) { + b" M src/main.c\n" + } else { + b"" + }; + return Ok(output(stdout)); + } + if cmd.args.iter().any(|arg| arg == "switch") { + let branch = cmd.args.last().unwrap().clone(); + if fail_switch_to.lock().unwrap().as_deref() == Some(branch.as_str()) { + return Ok(ShellOutput { + stdout: Vec::new(), + stderr: b"error: you need to resolve your current index first\n" + .to_vec(), + success: false, + }); + } + *head.lock().unwrap() = branch; + return Ok(output(b"")); + } + let current = format!("{}\n", head.lock().unwrap()); + Ok(output(current.as_bytes())) + }); + shell + } + } + + /// fs answers for a ready kernel tree with no active kw env: the + /// kernel-root probes pass, `.config` exists, `.kw/env.current` is + /// absent, `.kw/build.config` is unreadable (arch probes as None), + /// and there is no arch/ dir to glob images from. + fn expect_ready_tree(fs: &mut MockFileSystemTrait) { + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(|_| { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + }); + fs.expect_read_dir().returning(|_| { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + }); + } + + /// A ready kernel tree whose log dir can be created. + fn ready_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); + fs.expect_create_dir_all().returning(|_| Ok(())); + fs + } + + /// A ready kernel tree whose build produced an image and a + /// kernelrelease: build.config sets `arch=x86`, `arch/x86/boot/` + /// holds a bzImage, and `include/config/kernel.release` exists. The + /// image's metadata is unreadable, so its mtime falls back to the + /// epoch — still the only, hence newest, candidate. + fn built_tree_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(|path| { + if path.ends_with("build.config") { + Ok("arch=x86\n".to_string()) + } else if path.ends_with("kernel.release") { + Ok("6.17.0\n".to_string()) + } else { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + } + }); + fs.expect_read_dir().returning(|path| { + if path.ends_with("arch/x86/boot") { + Ok(vec![PathBuf::from( + "/home/user/linux/arch/x86/boot/bzImage", + )]) + } else { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + } + }); + fs.expect_metadata() + .returning(|_| Err(FileSystemError::IoError(io::Error::other("no metadata")))); + fs.expect_create_dir_all().returning(|_| Ok(())); + fs + } + + /// History answers for an actor whose jobs complete: no patchset + /// link, build-record writes accepted and dropped. + fn quiet_history() -> MockKwHistoryStore { + let mut history = MockKwHistoryStore::new(); + history + .expect_apply_record_for_branch() + .returning(|_, _| Ok(None)); + history.expect_record_build().returning(|_| Ok(())); + history + } + + /// A history double that captures written build records and answers + /// the patchset-link lookup with `apply_record`. + fn recording_history( + apply_record: Option, + ) -> (MockKwHistoryStore, Arc>>) { + let builds = Arc::new(Mutex::new(Vec::new())); + let builds_in_store = Arc::clone(&builds); + let mut history = MockKwHistoryStore::new(); + history + .expect_apply_record_for_branch() + .returning(move |_, _| Ok(apply_record.clone())); + history.expect_record_build().returning(move |record| { + builds_in_store.lock().unwrap().push(record); + Ok(()) + }); + (history, builds) + } + + /// Spawns the actor with every dependency explicit, a real temp log + /// dir, and the [`FakeProcess`] exposed so tests drive the "running" + /// process. + fn spawn_full_actor( test_name: &str, + history: MockKwHistoryStore, + shell: MockShellTrait, fs: MockFileSystemTrait, + env: MockEnvTrait, ) -> (KwHandle, Arc, PathBuf) { let process = Arc::new(FakeProcess::new()); let log_dir = tmp_log_dir(test_name); - let mut shell = MockShellTrait::new(); - shell.expect_execute().returning(|_| { - Ok(ShellOutput { - stdout: b"master\n".to_vec(), - stderr: Vec::new(), - success: true, - }) - }); let handle = KwActor::spawn( - Arc::new(MockKwHistoryStore::new()), + Arc::new(history), process.clone(), Arc::new(shell), Arc::new(fs), - Arc::new(MockEnvTrait::new()), + Arc::new(env), log_dir.clone(), ); (handle, process, log_dir) } + /// Spawns the actor with a real temp log dir and exposes the + /// [`FakeProcess`] so tests drive the "running" process. The env mock + /// has kw on PATH. + fn spawn_job_actor_with_mocks( + test_name: &str, + shell: MockShellTrait, + fs: MockFileSystemTrait, + ) -> (KwHandle, Arc, PathBuf) { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + spawn_full_actor(test_name, quiet_history(), shell, fs, env) + } + fn spawn_job_actor(test_name: &str) -> (KwHandle, Arc, PathBuf) { - let mut fs = MockFileSystemTrait::new(); - fs.expect_create_dir_all().returning(|_| Ok(())); - spawn_job_actor_with_fs(test_name, fs) + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + spawn_job_actor_with_mocks(test_name, shell, ready_fs()) } fn apply_record() -> KwApplyRecord { @@ -825,7 +1375,7 @@ mod tests { let spawned = process.spawned(); assert_eq!(1, spawned.len()); assert_eq!("kw", spawned[0].program); - assert_eq!(["build", "--alert=n"], spawned[0].args.as_slice()); + assert_eq!(["build"], spawned[0].args.as_slice()); assert_eq!(Path::new("/home/user/linux"), spawned[0].cwd); assert!(spawned[0].log_path.starts_with(&log_dir)); @@ -861,6 +1411,36 @@ mod tests { std::fs::remove_dir_all(&log_dir).unwrap(); } + #[tokio::test] + async fn start_build_merges_extra_args_into_the_spawned_argv() { + let (handle, process, log_dir) = spawn_job_actor("extra-args"); + + let mut request = start_request(); + request.extra_args = [ + "--verbose", + "--alert=vv", + "--save-log-to", + "/tmp/x.log", + "--ccache", + ] + .into_iter() + .map(String::from) + .collect(); + handle.start_build(request).await.unwrap(); + + let spawned = process.spawned(); + // Reserved options are stripped: the user's --alert and --save-log-to + // do not reach kw; the rest passes through in order. + assert_eq!( + ["build", "--verbose", "--ccache"].as_slice(), + spawned[0].args.as_slice() + ); + + process.last_child().finish(0); + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + #[tokio::test] async fn second_start_while_running_is_refused() { let (handle, process, log_dir) = spawn_job_actor("busy"); @@ -961,6 +1541,223 @@ mod tests { std::fs::remove_dir_all(&log_dir).unwrap(); } + #[tokio::test] + async fn start_build_refused_when_kw_binary_missing() { + let process = Arc::new(FakeProcess::new()); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| false); + let log_dir = tmp_log_dir("kw-missing"); + let handle = KwActor::spawn( + Arc::new(MockKwHistoryStore::new()), + process.clone(), + // No shell or fs calls are expected: the missing binary + // short-circuits the start before any other probe or spawn. + Arc::new(MockShellTrait::new()), + Arc::new(MockFileSystemTrait::new()), + Arc::new(env), + log_dir.clone(), + ); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::KwBinaryMissing)); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_tree_not_ready() { + let process = Arc::new(FakeProcess::new()); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(|_| { + Ok(ShellOutput { + stdout: b"kw, version 0.10.0\n".to_vec(), + stderr: Vec::new(), + success: true, + }) + }); + // The kernel-root probes pass, but there is no .kw directory: kw + // init was never run in this tree. + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir() + .returning(|path| path.file_name().is_none_or(|name| name != ".kw")); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + let log_dir = tmp_log_dir("tree-not-ready"); + let handle = KwActor::spawn( + Arc::new(MockKwHistoryStore::new()), + process.clone(), + Arc::new(shell), + Arc::new(fs), + Arc::new(env), + log_dir.clone(), + ); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!( + err, + KwStartError::TreeNotReady(TreeReadiness::MissingKwDir) + )); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_allowed_when_kw_version_below_floor() { + // kw's shipped VERSION file is stale (`beta-0.9` even at the 0.10 + // tag): a below-floor report warns but never gates the start. + let (shell, _calls) = recording_shell(b"kw, version beta-0.9\n", CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("version-below", shell, ready_fs()); + + handle.start_build(start_request()).await.unwrap(); + assert_eq!(1, process.spawned().len()); + + process.last_child().finish(0); + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_switches_to_requested_branch_before_spawning() { + let (shell, calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("checkout-order", shell, ready_fs()); + + handle.start_build(start_request()).await.unwrap(); + + { + let calls = calls.lock().unwrap(); + let git_position = |subcommand: &str| { + calls + .iter() + .position(|call| { + call.first().map(String::as_str) == Some("git") + && call.iter().any(|part| part == subcommand) + }) + .expect("expected git call missing") + }; + let status = git_position("status"); + let head = git_position("--show-current"); + let switch = git_position("switch"); + assert!( + status < head && head < switch, + "checkout policy must probe dirty state, then HEAD, then switch: {calls:?}" + ); + // Untracked scratch files don't block a build; only tracked + // changes do. + assert_eq!( + &calls[status], + &command(&[ + "git", + "-C", + "/home/user/linux", + "status", + "--porcelain", + "--untracked-files=no" + ]) + ); + // The `--` keeps a branch named like a flag from being parsed + // as one. + assert_eq!( + &calls[switch], + &command(&[ + "git", + "-C", + "/home/user/linux", + "switch", + "--", + "patchset-2026-08-01-17-30-00" + ]) + ); + } + assert_eq!(1, process.spawned().len()); + + process.last_child().finish(0); + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_worktree_is_dirty() { + let (shell, calls) = + recording_shell(KW_VERSION_OK, (b" M src/main.c\n", b"", true), SWITCH_OK); + let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); + let (handle, process, log_dir) = spawn_job_actor_with_mocks("dirty", shell, fs); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::DirtyWorktree)); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + // The refusal happens before any branch mutation. + assert!(!calls + .lock() + .unwrap() + .iter() + .any(|call| call.iter().any(|part| part == "switch"))); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_git_state_is_unverifiable() { + let (shell, _calls) = recording_shell( + KW_VERSION_OK, + (b"", b"fatal: not a git repository\n", false), + SWITCH_OK, + ); + let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); + let (handle, process, log_dir) = spawn_job_actor_with_mocks("git-probe-fail", shell, fs); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::GitStateProbe(_))); + assert!(err.to_string().contains("not a git repository")); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_branch_switch_fails() { + let (shell, _calls) = recording_shell( + KW_VERSION_OK, + CLEAN_STATUS, + ( + b"error: pathspec 'no-such-branch' did not match any file(s) known to git\n", + false, + ), + ); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("switch-fail", shell, ready_fs()); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::CheckoutFailed(_))); + assert!(err.to_string().contains("did not match")); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + #[tokio::test] async fn start_deploy_still_refused_until_the_deploy_step() { let handle = spawn_test_actor( @@ -1003,6 +1800,133 @@ mod tests { handle.shutdown().await; } + #[tokio::test] + async fn restore_switches_back_to_pre_job_branch_and_is_consumed() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + // The checkout policy left HEAD on the build branch. + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + // A successful restore consumes the context: a second restore has + // nothing to do. + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::NoRecordedBranch)); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn restore_refused_while_job_is_running() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-running", git.shell(), ready_fs()); + + handle.start_build(start_request()).await.unwrap(); + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::JobRunning)); + + // The context survives the refusal: restore works once the job + // ends. + process.last_child().finish(0); + let mut watch = handle.watch_status().await.unwrap(); + let _ = wait_for_terminal_status(&mut watch).await; + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn restore_refused_when_worktree_is_dirty() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-dirty", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + + git.set_dirty(true); + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::DirtyWorktree)); + // The refused restore did not touch the tree. + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + + // The context survives: clean the tree and retry. + git.set_dirty(false); + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn restore_failure_keeps_the_context_for_a_retry() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-fail", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + + git.fail_switches_to(Some("master")); + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::CheckoutFailed(_))); + assert!(err.to_string().contains("resolve your current index")); + + git.fail_switches_to(None); + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn refused_start_does_not_clobber_the_restore_context() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-clobber", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + + // This start is refused at spawn — after its HEAD probe and + // switch — and the switch is rolled back: the tree returns to the + // first job's branch and the recorded restore target is intact. + process.refuse_spawns(true); + let mut second = start_request(); + second.branch = "patchset-two".to_string(); + let err = handle.start_build(second).await.unwrap_err(); + assert!(matches!(err, KwStartError::Spawn(_))); + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + #[tokio::test] async fn shutdown_stops_actor() { let handle = spawn_test_actor( @@ -1021,19 +1945,54 @@ mod tests { #[tokio::test] async fn log_dir_creation_failure_refuses_start_and_stays_idle() { + let git = GitStub::on_branch("master"); let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); fs.expect_create_dir_all().returning(|_| { Err(FileSystemError::IoError(io::Error::other( "read-only filesystem", ))) }); - let (handle, process, log_dir) = spawn_job_actor_with_fs("log-dir-fail", fs); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("log-dir-fail", git.shell(), fs); let err = handle.start_build(start_request()).await.unwrap_err(); assert!(matches!(err, KwStartError::Fs(_))); assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); assert!(process.spawned().is_empty()); + // The switch happened and was rolled back: the tree is back on the + // user's branch, and no restore target was recorded. + assert_eq!(git.head(), "master"); + assert!(matches!( + handle.restore_previous_branch().await, + Err(KwError::NoRecordedBranch) + )); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn rollback_failure_keeps_the_spawn_refusal() { + let git = GitStub::on_branch("master"); + // Going forward works; coming back fails. + git.fail_switches_to(Some("master")); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("rollback-fails", git.shell(), ready_fs()); + process.refuse_spawns(true); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::Spawn(_))); + // The rollback failure is logged, not reported: the caller keeps + // the actionable refusal, and the tree honestly shows where HEAD + // is. No restore target was recorded. + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + assert!(matches!( + handle.restore_previous_branch().await, + Err(KwError::NoRecordedBranch) + )); handle.shutdown().await; std::fs::remove_dir_all(&log_dir).unwrap(); @@ -1116,4 +2075,299 @@ mod tests { handle.shutdown().await; std::fs::remove_dir_all(&log_dir).unwrap(); } + + #[tokio::test] + async fn successful_build_writes_a_full_build_record() { + let (history, builds) = recording_history(Some(apply_record())); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("build-record", history, shell, built_tree_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + let record = &builds[0]; + assert_eq!("mainline", record.kernel_tree_id); + assert_eq!("/home/user/linux", record.tree_path); + assert_eq!(Some("msg-1"), record.message_id.as_deref()); + assert_eq!("patchset-2026-08-01-17-30-00", record.branch); + assert_eq!(Some("x86"), record.arch.as_deref()); + assert_eq!( + Some("/home/user/linux/arch/x86/boot/bzImage"), + record.image_path.as_deref() + ); + assert_eq!(None, record.output_dir); + assert_eq!(Some("6.17.0"), record.kernelrelease.as_deref()); + assert!(record.log_path.starts_with(log_dir.to_str().unwrap())); + assert!(record.success); + // The readiness latest-lookup parses built_at as RFC3339. + assert!(chrono::DateTime::parse_from_rfc3339(&record.built_at).is_ok()); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn failed_build_writes_a_failure_record() { + let (history, builds) = recording_history(Some(apply_record())); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("failed-record", history, shell, built_tree_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(2); + let status = wait_for_terminal_status(&mut watch).await; + assert!(matches!( + status, + KwJobStatus::Failed { + exit_code: Some(2), + .. + } + )); + + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + let record = &builds[0]; + assert!(!record.success); + // Config/env facts are still recorded; what the build never + // produced is not — a stale image from an earlier build must + // not leak into a failure record. + assert_eq!(Some("x86"), record.arch.as_deref()); + assert_eq!(None, record.image_path); + assert_eq!(None, record.kernelrelease); + assert_eq!(Some("msg-1"), record.message_id.as_deref()); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn cancelled_build_writes_no_record() { + let (history, builds) = recording_history(None); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, _process, log_dir) = + spawn_full_actor("cancel-record", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + handle.cancel().await.unwrap(); + let status = wait_for_terminal_status(&mut watch).await; + + assert!(matches!(status, KwJobStatus::Cancelled { .. })); + assert!(builds.lock().unwrap().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn lost_exit_status_records_a_failed_build() { + let (history, builds) = recording_history(None); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("wait-failure", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + process.fail_waits(true); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + + // A lost exit status is an honest failure: the record must not + // become deploy-alone evidence. + assert!(matches!( + status, + KwJobStatus::Failed { + exit_code: None, + .. + } + )); + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + assert!(!builds[0].success); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn build_record_write_failure_keeps_the_terminal_status() { + let mut history = MockKwHistoryStore::new(); + history + .expect_apply_record_for_branch() + .returning(|_, _| Ok(None)); + history + .expect_record_build() + .returning(|_| Err(FileSystemError::IoError(io::Error::other("disk full")))); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("record-write-fails", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + + // The build's real outcome reached the user; a history-write + // failure must not turn it into a reported failure. + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn build_record_keeps_no_patchset_link_when_apply_lookup_fails() { + let builds = Arc::new(Mutex::new(Vec::new())); + let builds_in_store = Arc::clone(&builds); + let mut history = MockKwHistoryStore::new(); + history.expect_apply_record_for_branch().returning(|_, _| { + Err(FileSystemError::IoError(io::Error::other( + "corrupt history", + ))) + }); + history.expect_record_build().returning(move |record| { + builds_in_store.lock().unwrap().push(record); + Ok(()) + }); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("link-lookup-fails", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + + // The lookup error must not drop the record, only the link. + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + assert_eq!(None, builds[0].message_id); + assert!(builds[0].success); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn successful_build_with_active_env_records_the_output_dir() { + // env.current is read once, at accept time. The completion record + // must describe the env the build ran under — the snapshot — not + // the tree's env state at whatever time the job ends. + let env_current_reads = Arc::new(AtomicU64::new(0)); + let env_current_reads_in_fs = Arc::clone(&env_current_reads); + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file().returning(|_| true); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(move |path| { + if path.ends_with("env.current") { + env_current_reads_in_fs.fetch_add(1, Ordering::SeqCst); + Ok("testenv\n".to_string()) + } else if path.ends_with("build.config") { + Ok("arch=x86\n".to_string()) + } else if path.ends_with("kernel.release") { + Ok("6.17.0\n".to_string()) + } else { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + } + }); + // Every boot-dir probe answers with an image inside the probed + // dir: the record's image path shows which build root was used. + fs.expect_read_dir().returning(|path| { + if path.ends_with("arch/x86/boot") { + Ok(vec![path.join("bzImage")]) + } else { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + } + }); + fs.expect_metadata() + .returning(|_| Err(FileSystemError::IoError(io::Error::other("no metadata")))); + fs.expect_create_dir_all().returning(|_| Ok(())); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env.expect_var() + .returning(|_| Ok("/home/user/.cache".to_string())); + let (history, builds) = recording_history(None); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = spawn_full_actor("env-build", history, shell, fs, env); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + let record = &builds[0]; + let output_dir = record + .output_dir + .as_deref() + .expect("an env build records its O= dir"); + assert!( + output_dir.contains("/kw/envs/") && output_dir.ends_with("/testenv"), + "unexpected output dir: {output_dir}" + ); + let image = record.image_path.as_deref().expect("image recorded"); + assert!( + image.starts_with(output_dir) && image.ends_with("bzImage"), + "image {image} must be probed under the env's output dir {output_dir}" + ); + assert_eq!(Some("x86"), record.arch.as_deref()); + assert_eq!(Some("6.17.0"), record.kernelrelease.as_deref()); + assert!(record.success); + } + // Read at accept time only: the completion record uses the + // snapshot, never a re-resolve. + assert_eq!(1, env_current_reads.load(Ordering::SeqCst)); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } } diff --git a/src/kw/argv.rs b/src/kw/argv.rs new file mode 100644 index 0000000..1870364 --- /dev/null +++ b/src/kw/argv.rs @@ -0,0 +1,187 @@ +//! kw argv construction: patch-hub's base command lines plus the user +//! extra-args merge in which reserved options always win. + +// Production caller is the unix-only actor. +#![cfg_attr(not(unix), allow(dead_code))] + +/// A CLI option patch-hub controls: user-supplied extra args that set it +/// are stripped, so the occurrence on patch-hub's own base argv wins. +pub struct ReservedOption { + /// Every spelling of the option, e.g. `&["--force", "-f"]`. + spellings: &'static [&'static str], + /// Whether the option takes a value (`--name=value`, or `--name + /// value` as a separate token). + takes_value: bool, +} + +impl ReservedOption { + pub const fn new(spellings: &'static [&'static str], takes_value: bool) -> Self { + Self { + spellings, + takes_value, + } + } +} + +/// Reserved for `kw build`: patch-hub owns the job's log file +/// (`ProcessTrait` captures kw's stdout/stderr to it) — a user-supplied +/// `--save-log-to` would split stdout/stderr away from the job log — and +/// never runs `--menu` from automation: the job's stdio is a log file, so +/// menuconfig would hang until cancelled. +/// +/// `--alert` is stripped from extras (and not injected on the base argv): +/// kw beta-0.9 (still what many installs report, including this lab) treats +/// unrecognized options as hard failures (`Invalid option`), and the +/// unattended default is already `alert=n` in kw's own config. +const BUILD_RESERVED: &[ReservedOption] = &[ + ReservedOption::new(&["--alert"], true), + ReservedOption::new(&["--save-log-to"], true), + ReservedOption::new(&["--menu"], false), +]; + +/// The argv for a build job: `kw build ` (reserved extras stripped). +pub fn build_argv(extra_args: &[String]) -> Vec { + merge_extra_args(&["build"], BUILD_RESERVED, extra_args) +} + +/// Appends user-supplied extra args to `base`, stripping every token that +/// would override a reserved option: `--name=value` is stripped whole, +/// `--name value` consumes the following token too, and a boolean +/// reserved option strips only itself. All other extras pass through in +/// order. +pub fn merge_extra_args( + base: &[&str], + reserved: &[ReservedOption], + extra_args: &[String], +) -> Vec { + let mut argv: Vec = base.iter().map(|arg| arg.to_string()).collect(); + let mut extras = extra_args.iter(); + while let Some(token) = extras.next() { + match reserved_option_for(reserved, token) { + Some(option) => { + if option.takes_value && !token.contains('=') { + // `--name value`: the separate value token goes too. + extras.next(); + } + } + None => argv.push(token.clone()), + } + } + argv +} + +/// Finds the reserved option a token sets, if any: an exact spelling +/// match, or `--name=value` for a long spelling. The `=` boundary keeps +/// `--alertness` from matching `--alert`. +fn reserved_option_for<'a>( + reserved: &'a [ReservedOption], + token: &str, +) -> Option<&'a ReservedOption> { + reserved.iter().find(|option| { + option.spellings.iter().any(|spelling| { + token == *spelling + || spelling.starts_with("--") + && token + .strip_prefix(spelling) + .is_some_and(|rest| rest.starts_with('=')) + }) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn extras(tokens: &[&str]) -> Vec { + tokens.iter().map(|token| token.to_string()).collect() + } + + #[test] + fn build_argv_without_extras_is_the_base_command() { + assert_eq!(vec!["build"], build_argv(&[])); + } + + #[test] + fn extras_are_appended_in_order() { + assert_eq!( + vec!["build", "--verbose", "--ccache", "-j8"], + build_argv(&extras(&["--verbose", "--ccache", "-j8"])) + ); + } + + #[test] + fn reserved_alert_is_stripped_from_extras() { + assert_eq!( + vec!["build", "--verbose"], + build_argv(&extras(&["--alert=vv", "--verbose"])) + ); + // Separate-token value form: the value token is consumed too. + assert_eq!( + vec!["build", "--verbose"], + build_argv(&extras(&["--alert", "v", "--verbose"])) + ); + } + + #[test] + fn save_log_to_is_reserved_in_both_value_forms() { + assert_eq!( + vec!["build"], + build_argv(&extras(&["--save-log-to=/tmp/x.log"])) + ); + assert_eq!( + vec!["build"], + build_argv(&extras(&["--save-log-to", "/tmp/x.log"])) + ); + } + + #[test] + fn similar_prefix_is_not_reserved() { + // `--alertness` only shares a prefix with `--alert`. + assert_eq!( + vec!["build", "--alertness"], + build_argv(&extras(&["--alertness"])) + ); + } + + #[test] + fn trailing_reserved_option_without_value_is_stripped() { + assert_eq!(vec!["build"], build_argv(&extras(&["--alert"]))); + } + + #[test] + fn menu_is_stripped_without_eating_the_next_token() { + // kw build --menu would open menuconfig with the job's stdio + // redirected to a log file — a hang, not a build. + assert_eq!( + vec!["build", "--verbose"], + build_argv(&extras(&["--menu", "--verbose"])) + ); + } + + #[test] + fn boolean_reserved_strips_only_itself_in_all_spellings() { + // A boolean reserved option must not eat the following token. + let reserved = [ReservedOption::new(&["--force", "-f"], false)]; + assert_eq!( + vec!["deploy", "extra"], + merge_extra_args(&["deploy"], &reserved, &extras(&["--force", "extra"])) + ); + assert_eq!( + vec!["deploy", "extra"], + merge_extra_args(&["deploy"], &reserved, &extras(&["-f", "extra"])) + ); + } + + #[test] + fn tokens_after_a_consumed_value_keep_flowing() { + let reserved = [ReservedOption::new(&["--remote"], true)]; + assert_eq!( + vec!["deploy", "--no-reboot"], + merge_extra_args( + &["deploy"], + &reserved, + &extras(&["--remote", "host:22", "--no-reboot"]) + ) + ); + } +} diff --git a/src/kw/errors.rs b/src/kw/errors.rs index 30179b4..1b6f434 100644 --- a/src/kw/errors.rs +++ b/src/kw/errors.rs @@ -7,7 +7,7 @@ use thiserror::Error; use crate::infrastructure::process::ProcessError; use crate::{ infrastructure::{file_system::FileSystemError, shell::ShellError}, - kw::readiness::KwReadinessError, + kw::readiness::{KwReadinessError, TreeReadiness}, }; #[derive(Debug, Error)] @@ -18,6 +18,14 @@ pub enum KwError { NoJobRunning, #[error("no pre-job branch was recorded")] NoRecordedBranch, + #[error("a kw job is running; restore the branch after it finishes")] + JobRunning, + #[error("the kernel tree has staged and/or unstaged changes; commit or stash them first")] + DirtyWorktree, + #[error("could not verify the kernel tree's git state: {0}")] + GitStateProbe(String), + #[error("failed to switch the kernel tree back to the previous branch: {0}")] + CheckoutFailed(String), #[error("history error: {0}")] History(#[from] FileSystemError), #[error("readiness error: {0}")] @@ -35,6 +43,18 @@ pub enum KwStartError { ActorUnavailable(String), #[error("a kw job is already running")] JobAlreadyRunning, + #[error("kw binary not found on PATH; install kw and make sure it is on PATH")] + KwBinaryMissing, + #[error("the kernel tree is not ready: {0}")] + TreeNotReady(TreeReadiness), + #[error("could not resolve the kw env state: {0}")] + Readiness(#[from] KwReadinessError), + #[error("the kernel tree has staged and/or unstaged changes; commit or stash them first")] + DirtyWorktree, + #[error("could not verify the kernel tree's git state: {0}")] + GitStateProbe(String), + #[error("failed to switch the kernel tree to the requested branch: {0}")] + CheckoutFailed(String), #[error("kw jobs are not supported yet")] NotImplemented, // Spawning a process is unix-only (ProcessTrait is cfg(unix)). @@ -44,3 +64,32 @@ pub enum KwStartError { #[error("filesystem error: {0}")] Fs(#[from] FileSystemError), } + +/// Shared git-state refusal for the start and restore paths, converted +/// into the public error each path reports. +#[derive(Debug)] +pub(crate) enum TreeGitError { + DirtyWorktree, + Probe(String), + Switch(String), +} + +impl From for KwStartError { + fn from(error: TreeGitError) -> Self { + match error { + TreeGitError::DirtyWorktree => KwStartError::DirtyWorktree, + TreeGitError::Probe(detail) => KwStartError::GitStateProbe(detail), + TreeGitError::Switch(detail) => KwStartError::CheckoutFailed(detail), + } + } +} + +impl From for KwError { + fn from(error: TreeGitError) -> Self { + match error { + TreeGitError::DirtyWorktree => KwError::DirtyWorktree, + TreeGitError::Probe(detail) => KwError::GitStateProbe(detail), + TreeGitError::Switch(detail) => KwError::CheckoutFailed(detail), + } + } +} diff --git a/src/kw/history.rs b/src/kw/history.rs index ed95037..85d35c5 100644 --- a/src/kw/history.rs +++ b/src/kw/history.rs @@ -80,9 +80,25 @@ pub trait KwHistoryStore: Send + Sync { kernel_tree_id: &str, ) -> Result, FileSystemError>; + /// Returns the newest apply record for the tree whose applied branch + /// is `branch` — the link from a build's branch back to the patchset + /// it came from. A missing history file is a normal state, not an + /// error. Records with unparseable `applied_at` values sort oldest, + /// same convention as the build records. + // The only production caller is the unix-only actor's build-record + // writer. + #[cfg_attr(not(unix), allow(dead_code))] + fn apply_record_for_branch( + &self, + kernel_tree_id: &str, + branch: &str, + ) -> Result, FileSystemError>; + /// Inserts or replaces the build record for the record's /// `(kernel_tree_id, branch)` pair. - #[allow(dead_code)] + // The only production caller is the unix-only actor's build-record + // writer. + #[cfg_attr(not(unix), allow(dead_code))] fn record_build(&self, record: KwBuildRecord) -> Result<(), FileSystemError>; /// Returns the build record for the `(kernel_tree_id, branch)` pair, or @@ -106,7 +122,6 @@ pub trait KwHistoryStore: Send + Sync { /// Returns the record for `(kernel_tree_id, branch)` and the newest /// record for the tree across branches from a single load of the /// history file — the pair a readiness snapshot is computed from. - #[allow(dead_code)] fn build_records( &self, kernel_tree_id: &str, @@ -194,6 +209,25 @@ impl KwHistoryStore for FileKwHistoryStore { .map_err(|e| self.error_with_path(&self.apply_history_path, e)) } + fn apply_record_for_branch( + &self, + kernel_tree_id: &str, + branch: &str, + ) -> Result, FileSystemError> { + self.load_records(&self.apply_history_path) + .map(|records: ApplyRecords| { + records + .values() + .filter_map(|by_tree| by_tree.get(kernel_tree_id)) + .filter(|record| record.applied_branch == branch) + .max_by_key(|record| { + chrono::DateTime::parse_from_rfc3339(&record.applied_at).ok() + }) + .cloned() + }) + .map_err(|e| self.error_with_path(&self.apply_history_path, e)) + } + fn record_build(&self, record: KwBuildRecord) -> Result<(), FileSystemError> { self.store_build_record(record) .map_err(|e| self.error_with_path(&self.build_history_path, e)) @@ -284,6 +318,18 @@ mod tests { } } + fn record_at( + message_id: &str, + kernel_tree_id: &str, + branch: &str, + applied_at: &str, + ) -> KwApplyRecord { + KwApplyRecord { + applied_at: applied_at.to_string(), + ..record(message_id, kernel_tree_id, branch) + } + } + fn build(kernel_tree_id: &str, branch: &str, built_at: &str) -> KwBuildRecord { KwBuildRecord { kernel_tree_id: kernel_tree_id.to_string(), @@ -379,6 +425,126 @@ mod tests { fs::remove_dir_all(&dir).unwrap(); } + #[test] + fn apply_record_for_branch_finds_record_across_message_ids() { + let dir = tmp_dir("branch-lookup"); + let store = store_at(&dir); + + store + .record_apply(record("msg-1", "mainline", "patchset-x")) + .unwrap(); + store + .record_apply(record("msg-2", "mainline", "patchset-y")) + .unwrap(); + + assert_eq!( + Some(record("msg-2", "mainline", "patchset-y")), + store + .apply_record_for_branch("mainline", "patchset-y") + .unwrap() + ); + assert_eq!( + None, + store + .apply_record_for_branch("mainline", "never-applied") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn apply_record_for_branch_scopes_to_the_tree() { + let dir = tmp_dir("branch-tree-scope"); + let store = store_at(&dir); + + // The same branch name applied to two trees resolves per tree. + store + .record_apply(record("msg-1", "mainline", "patchset-x")) + .unwrap(); + store + .record_apply(record("msg-2", "stable", "patchset-x")) + .unwrap(); + + assert_eq!( + Some(record("msg-2", "stable", "patchset-x")), + store + .apply_record_for_branch("stable", "patchset-x") + .unwrap() + ); + assert_eq!( + None, + store + .apply_record_for_branch("amd-gfx", "patchset-x") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn apply_record_for_branch_returns_newest_reapply() { + let dir = tmp_dir("branch-newest"); + let store = store_at(&dir); + + // Two patchsets applied onto the same branch name: the newest + // applied_at wins, and unparseable timestamps sort oldest (the + // build records' convention). + store + .record_apply(record_at( + "msg-old", + "mainline", + "patchset-x", + "2026-08-01T10:00:00Z", + )) + .unwrap(); + store + .record_apply(record_at( + "msg-new", + "mainline", + "patchset-x", + "2026-08-02T10:00:00Z", + )) + .unwrap(); + store + .record_apply(record_at( + "msg-broken", + "mainline", + "patchset-x", + "not a timestamp", + )) + .unwrap(); + + assert_eq!( + Some(record_at( + "msg-new", + "mainline", + "patchset-x", + "2026-08-02T10:00:00Z" + )), + store + .apply_record_for_branch("mainline", "patchset-x") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn apply_record_for_branch_missing_history_reads_as_none() { + let dir = tmp_dir("branch-missing"); + let store = store_at(&dir); + + assert_eq!( + None, + store + .apply_record_for_branch("mainline", "patchset-x") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn record_creates_parent_directories() { let dir = tmp_dir("parents"); diff --git a/src/kw/messages.rs b/src/kw/messages.rs index 067b6da..44a067f 100644 --- a/src/kw/messages.rs +++ b/src/kw/messages.rs @@ -20,8 +20,13 @@ use crate::{ pub struct StartRequest { pub kernel_tree_id: String, pub tree: KernelTree, - /// Branch the job must run on. + /// Branch the job must run on; the actor switches the tree onto it + /// before spawning and leaves HEAD there after the job. pub branch: String, + /// Extra kw CLI tokens, already whitespace-split by the caller. + /// Reserved options (`--alert`, `--save-log-to`) are stripped — + /// patch-hub's own argv wins. + pub extra_args: Vec, } pub enum KwMessage { diff --git a/src/kw/mod.rs b/src/kw/mod.rs index 24f19ea..d5f0da2 100644 --- a/src/kw/mod.rs +++ b/src/kw/mod.rs @@ -3,6 +3,7 @@ #[cfg(unix)] pub mod actor; +pub mod argv; pub mod errors; pub mod handle; pub mod history; diff --git a/src/kw/readiness.rs b/src/kw/readiness.rs index 28f3d46..e577c0f 100644 --- a/src/kw/readiness.rs +++ b/src/kw/readiness.rs @@ -114,7 +114,6 @@ pub fn is_kernel_root(fs: &dyn FileSystemTrait, path: &Path) -> bool { /// refuses to activate an env while an in-tree `.config` exists /// (`kw_env.sh::validate_env_before_switch`), so with an env active the /// `.config` lives only at the env's `O=` dir. -#[allow(dead_code)] pub fn probe_tree( fs: &dyn FileSystemTrait, tree_path: &Path, @@ -177,7 +176,6 @@ pub fn read_build_arch(fs: &dyn FileSystemTrait, tree_path: &Path) -> Option SystemTime { .unwrap_or(SystemTime::UNIX_EPOCH) } +/// Reads the built kernel's release string from +/// `/include/config/kernel.release`, the file a kernel build +/// generates — cheaper than re-running `make kernelrelease`, and `None` +/// when the build never produced one (or produced an empty one). +// The only production caller is the unix-only actor's build-record +// writer. +#[cfg_attr(not(unix), allow(dead_code))] +pub fn read_kernelrelease(fs: &dyn FileSystemTrait, build_root: &Path) -> Option { + let release = fs + .read_to_string( + &build_root + .join("include") + .join("config") + .join("kernel.release"), + ) + .ok()?; + let release = release.trim(); + if release.is_empty() { + None + } else { + Some(release.to_string()) + } +} + /// Minimum kw version this integration is verified against. pub const KW_MIN_VERSION: (u32, u32) = (0, 10); @@ -427,7 +448,6 @@ pub enum DeployAloneRefusal { /// about the tree's *current* state. [`evaluate_readiness`] conjoins /// [`TreeReadiness`] into its `deploy_alone` verdict; prefer it over /// calling this directly. -#[allow(dead_code)] pub fn check_deploy_alone( record: Option<&KwBuildRecord>, tree: &KernelTree, @@ -487,7 +507,8 @@ pub struct KwReadiness { /// Runs all readiness probes for `tree` and composes them into a /// [`KwReadiness`] snapshot. `head_branch` is the tree's current branch — /// resolving it (via git) is the caller's job, keeping these probes pure. -#[allow(dead_code)] +// The only caller is the unix-only actor's GetReadiness. +#[cfg_attr(not(unix), allow(dead_code))] pub fn evaluate_readiness( fs: &dyn FileSystemTrait, env: &dyn EnvTrait, @@ -1039,6 +1060,31 @@ last_line_without_newline=yes"; ); } + #[test] + fn kernelrelease_reads_and_trims_the_release_file() { + let dir = make_ready_tree("kernelrelease"); + let config_dir = dir.path().join("include").join("config"); + fs::create_dir_all(&config_dir).unwrap(); + fs::write(config_dir.join("kernel.release"), "6.17.0-rc1\n").unwrap(); + + assert_eq!( + Some("6.17.0-rc1".to_string()), + read_kernelrelease(&OsFileSystem, dir.path()) + ); + } + + #[test] + fn kernelrelease_is_none_without_a_release_file() { + let dir = make_ready_tree("kernelrelease-missing"); + + assert_eq!(None, read_kernelrelease(&OsFileSystem, dir.path())); + + let config_dir = dir.path().join("include").join("config"); + fs::create_dir_all(&config_dir).unwrap(); + fs::write(config_dir.join("kernel.release"), "\n").unwrap(); + assert_eq!(None, read_kernelrelease(&OsFileSystem, dir.path())); + } + fn shell_output(stdout: &str, success: bool) -> ShellOutput { ShellOutput { stdout: stdout.as_bytes().to_vec(),