From cbcc0dad628d14b7c828431a47f47e23435c82e0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 15:48:51 -0400 Subject: [PATCH 1/2] fix(test): eliminate session lock race and openssl /dev/stdin flake Two flake sources identified across 100+ full-suite runs: 1. Session lock EINVAL race: cleanup_session_root's remove_dir_all could delete the session root between with_session_lock's create_dir_all and File::create, causing EINVAL. Fix: retry the create-dir + create-file sequence as a unit. 2. mTLS cert generation: openssl req -key /dev/stdin failed under fd pressure with "Bad file descriptor". Fix: read from the already- written server.key file path instead of piping through /dev/stdin. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/install.rs | 7 ++++--- lib/crates/fabro-test/src/lib.rs | 20 +++++++++++++++----- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 39f3d6a4b..a425daacc 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -139,16 +139,17 @@ fn generate_mtls_certs(dir: &Path) -> Result<()> { let server_key_path = dir.join("server.key"); std::fs::write(&server_key_path, &server_key)?; - let csr = run_openssl_with_stdin( + let csr = run_openssl( &[ "req", "-new", "-key", - "/dev/stdin", + server_key_path + .to_str() + .context("server key path is not valid UTF-8")?, "-subj", "/CN=localhost", ], - &server_key, "generate server CSR", )?; diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 28aab0c78..44efc1c0b 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -262,13 +262,23 @@ fn ensure_parent_dir(path: &Path) { } fn with_session_lock(root: &Path, f: impl FnOnce() -> T) -> T { - std::fs::create_dir_all(root) - .unwrap_or_else(|err| panic!("failed to create {}: {err}", root.display())); let lock_path = session_lock_path(root); - ensure_parent_dir(&lock_path); - let lock_file = File::create(&lock_path) - .unwrap_or_else(|err| panic!("failed to create {}: {err}", lock_path.display())); + // Retry create-dir + create-file as a unit: another process's + // cleanup_session_root can remove_dir_all between the two calls. let deadline = std::time::Instant::now() + SESSION_LOCK_TIMEOUT; + let lock_file = loop { + std::fs::create_dir_all(root) + .unwrap_or_else(|err| panic!("failed to create {}: {err}", root.display())); + ensure_parent_dir(&lock_path); + match File::create(&lock_path) { + Ok(f) => break f, + Err(err) if std::time::Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(10)); + continue; + } + Err(err) => panic!("failed to create {}: {err}", lock_path.display()), + } + }; while !fabro_proc::try_flock_exclusive(&lock_file) .unwrap_or_else(|err| panic!("failed to lock {}: {err}", lock_path.display())) { From a54e27f42ea697865d11e3116ff4005aeda8009b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 15:56:51 -0400 Subject: [PATCH 2/2] fix: resolve clippy warnings across workspace - fabro-types: remove redundant "freeform" match arm (match_same_arms) - fabro-server: use let...else and remove needless return - fabro-cli/runner: use while-let instead of match loop, unwrap Option from build_artifact_uploader return type - fabro-cli/attach: introduce AttachOptions struct to reduce bool parameter count (fn_params_excessive_bools) - fabro-test: fix unused variable and needless continue in session lock Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fabro-cli/src/commands/run/attach.rs | 44 +++++++++++-------- .../fabro-cli/src/commands/run/runner.rs | 28 +++++------- lib/crates/fabro-server/src/server.rs | 13 ++---- lib/crates/fabro-test/src/lib.rs | 3 +- lib/crates/fabro-types/src/interview.rs | 1 - 5 files changed, 41 insertions(+), 48 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/run/attach.rs b/lib/crates/fabro-cli/src/commands/run/attach.rs index 417cf80b6..3b3a74ddf 100644 --- a/lib/crates/fabro-cli/src/commands/run/attach.rs +++ b/lib/crates/fabro-cli/src/commands/run/attach.rs @@ -91,17 +91,26 @@ pub(crate) async fn attach_run_with_client( attach_live_run_with_client( client, run_id, - auto_approve, - verbose, replay_events, stream, - kill_on_detach, styles, - json_output, + AttachOptions { + auto_approve, + verbose, + kill_on_detach, + json_output, + }, ) .await } +struct AttachOptions { + auto_approve: bool, + verbose: bool, + kill_on_detach: bool, + json_output: bool, +} + fn replay_run_with_client( verbose: bool, events: Vec, @@ -124,31 +133,28 @@ fn replay_run_with_client( async fn attach_live_run_with_client( client: &server_client::ServerStoreClient, run_id: &RunId, - auto_approve: bool, - verbose: bool, existing_events: Vec, mut stream: server_client::RunAttachEventStream, - kill_on_detach: bool, styles: &'static Styles, - json_output: bool, + opts: AttachOptions, ) -> Result { let is_tty = std::io::stderr().is_terminal(); - let mut progress_ui = run_progress::ProgressUI::new(is_tty, verbose); + let mut progress_ui = run_progress::ProgressUI::new(is_tty, opts.verbose); let ctrl_c_signal = ctrl_c(); tokio::pin!(ctrl_c_signal); for event in existing_events { let line = event_payload_line(&event)?; - emit_progress_line(&mut progress_ui, &line, json_output)?; + emit_progress_line(&mut progress_ui, &line, opts.json_output)?; } if let Some(exit_code) = handle_pending_server_interview( client, run_id, - auto_approve, + opts.auto_approve, &mut progress_ui, styles, - json_output, + opts.json_output, ) .await? { @@ -158,23 +164,23 @@ async fn attach_live_run_with_client( loop { let next_event = tokio::select! { _ = &mut ctrl_c_signal => { - handle_detach_signal(client, run_id, kill_on_detach).await; - finish_progress(&mut progress_ui, json_output); + handle_detach_signal(client, run_id, opts.kill_on_detach).await; + finish_progress(&mut progress_ui, opts.json_output); return Ok(ExitCode::from(1)); } result = stream.next_event() => result?, }; let Some(event) = next_event else { - finish_progress(&mut progress_ui, json_output); + finish_progress(&mut progress_ui, opts.json_output); return Err(anyhow::anyhow!(ATTACH_PREMATURE_EOF_MESSAGE)); }; let line = event_payload_line(&event)?; - emit_progress_line(&mut progress_ui, &line, json_output)?; + emit_progress_line(&mut progress_ui, &line, opts.json_output)?; if let Some(exit_code) = event_exit_code(&event) { - finish_progress(&mut progress_ui, json_output); + finish_progress(&mut progress_ui, opts.json_output); return Ok(exit_code); } @@ -182,10 +188,10 @@ async fn attach_live_run_with_client( if let Some(exit_code) = handle_pending_server_interview( client, run_id, - auto_approve, + opts.auto_approve, &mut progress_ui, styles, - json_output, + opts.json_output, ) .await? { diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index a4718e8bb..c5b711439 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -63,8 +63,11 @@ pub(crate) async fn execute( .run .as_ref() .ok_or_else(|| anyhow!("Run {run_id} has no run record in store"))?; - let artifact_uploader = - build_artifact_uploader(run_id, client.clone_for_reuse(), artifact_upload_token); + let artifact_uploader = Some(build_artifact_uploader( + run_id, + client.clone_for_reuse(), + artifact_upload_token, + )); let interviewer = Arc::new(ControlInterviewer::new()); let cancel_token = Arc::new(AtomicBool::new(false)); tokio::spawn(read_worker_control_stream( @@ -115,17 +118,10 @@ async fn read_worker_control_stream( R: AsyncRead + Unpin, { let mut lines = BufReader::new(reader).lines(); - loop { - match lines.next_line().await { - Ok(Some(line)) => { - apply_worker_control_line(&interviewer, &cancel_token, &line).await; - } - Ok(None) | Err(_) => { - interviewer.abort_all().await; - break; - } - } + while let Ok(Some(line)) = lines.next_line().await { + apply_worker_control_line(&interviewer, &cancel_token, &line).await; } + interviewer.abort_all().await; } async fn apply_worker_control_line( @@ -156,17 +152,15 @@ fn build_artifact_uploader( run_id: RunId, client: server_client::ServerStoreClient, artifact_upload_token: Option, -) -> Option> { - let uploader: Arc = match artifact_upload_token { +) -> Arc { + match artifact_upload_token { Some(token) => Arc::new(HttpArtifactUploader { run_id, client, bearer_token: token, }), None => Arc::new(MissingArtifactUploadTokenUploader { run_id }), - }; - - Some(uploader) + } } struct HttpArtifactUploader { diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 3d22c4a01..6cbf171e2 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -502,16 +502,11 @@ impl SlackService { return; }; - let pending = match load_pending_interview(state.as_ref(), run_id, &submission.qid).await { - Ok(pending) => pending, - Err(_) => return, - }; - if submit_pending_interview_answer(state.as_ref(), &pending, submission.answer) - .await - .is_err() - { + let Ok(pending) = load_pending_interview(state.as_ref(), run_id, &submission.qid).await + else { return; - } + }; + let _ = submit_pending_interview_answer(state.as_ref(), &pending, submission.answer).await; } } diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 44efc1c0b..b058cc5bf 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -272,9 +272,8 @@ fn with_session_lock(root: &Path, f: impl FnOnce() -> T) -> T { ensure_parent_dir(&lock_path); match File::create(&lock_path) { Ok(f) => break f, - Err(err) if std::time::Instant::now() < deadline => { + Err(_) if std::time::Instant::now() < deadline => { std::thread::sleep(Duration::from_millis(10)); - continue; } Err(err) => panic!("failed to create {}: {err}", lock_path.display()), } diff --git a/lib/crates/fabro-types/src/interview.rs b/lib/crates/fabro-types/src/interview.rs index a32e62d3a..d3d42ddfd 100644 --- a/lib/crates/fabro-types/src/interview.rs +++ b/lib/crates/fabro-types/src/interview.rs @@ -22,7 +22,6 @@ impl InterviewQuestionType { "yes_no" => Self::YesNo, "multiple_choice" => Self::MultipleChoice, "multi_select" => Self::MultiSelect, - "freeform" => Self::Freeform, "confirmation" => Self::Confirmation, _ => Self::Freeform, }