Skip to content

Commit 3ded49e

Browse files
fix: bound Conda and Poetry subprocess probes (#555)
Use the shared bounded capture runner for Conda info and Poetry environment/configuration probes, removing their unbounded output waits. - Apply the existing15-second execution deadline and combined4MiB captured-output limit to every manager call, preserving arguments and workspace cwd. - Distinguish missing default Conda from real execution failures; reject invalid JSON/UTF-8 and malformed Poetry boolean settings explicitly. - Preserve existing textual Poetry path parsing and document the exact per-process/direct-child boundary. - Add cross-platform contract tests and real noisy/hanging manager fixtures with no unmanaged fixture descendants. Validation:92 Windows and101 Linux affected-crate unit tests passed, mandatory formatting/Clippy and workspace all-target/all-feature lint clean, independent Reviewer clean after fixes. Refs #530. This advances manager deadlines; it deliberately does not close the remaining process-tree/active-shutdown ownership work in #530/#529. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 39c6710 commit 3ded49e

6 files changed

Lines changed: 508 additions & 111 deletions

File tree

‎crates/pet-conda/src/conda_info.rs‎

Lines changed: 217 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,16 @@
33

44
use log::{error, trace, warn};
55
use pet_fs::path::resolve_symlink;
6-
use pet_python_utils::executable::new_silent_command;
7-
use std::path::PathBuf;
6+
use pet_python_utils::{
7+
executable::new_silent_command,
8+
process::{output, ProcessError, DEFAULT_TIMEOUT},
9+
};
10+
use std::{
11+
io,
12+
path::PathBuf,
13+
process::{Command, Output},
14+
time::Duration,
15+
};
816

917
#[derive(Debug, serde::Deserialize)]
1018
pub struct CondaInfo {
@@ -37,7 +45,14 @@ pub struct CondaInfoJson {
3745

3846
impl CondaInfo {
3947
pub fn from(executable: Option<PathBuf>) -> Option<CondaInfo> {
40-
// let using_default = executable.is_none() || executable == Some("conda".into());
48+
Self::from_with_runner(executable, output)
49+
}
50+
51+
fn from_with_runner(
52+
executable: Option<PathBuf>,
53+
run: impl FnOnce(&mut Command, Duration) -> Result<Output, ProcessError>,
54+
) -> Option<Self> {
55+
let using_default = executable.is_none();
4156
// Possible we got a symlink to the conda exe, first try to resolve that.
4257
let executable = if cfg!(windows) {
4358
executable.clone().unwrap_or("conda".into())
@@ -46,16 +61,15 @@ impl CondaInfo {
4661
resolve_symlink(&executable).unwrap_or(executable)
4762
};
4863

49-
let result = new_silent_command(&executable)
50-
.arg("info")
51-
.arg("--json")
52-
.output();
53-
trace!("Executing Conda: {:?} info --json -a", executable);
64+
let result = run(
65+
new_silent_command(&executable).args(["info", "--json"]),
66+
DEFAULT_TIMEOUT,
67+
);
68+
trace!("Executed Conda: {:?} info --json", executable);
5469
match result {
5570
Ok(output) => {
5671
if output.status.success() {
57-
let output = String::from_utf8_lossy(&output.stdout).to_string();
58-
match serde_json::from_str::<CondaInfoJson>(output.trim()) {
72+
match serde_json::from_slice::<CondaInfoJson>(&output.stdout) {
5973
Ok(info) => {
6074
let envs_path = info.envs_path.unwrap_or_default();
6175
let mut envs_dirs = info.envs_dirs.unwrap_or_default();
@@ -84,25 +98,206 @@ impl CondaInfo {
8498
}
8599
} else {
86100
let stderr = String::from_utf8_lossy(&output.stderr).to_string();
87-
// No point logging the message if conda is not installed or a custom conda exe wasn't provided.
88-
if executable.to_string_lossy() != "conda" {
89-
warn!(
90-
"Failed to get conda info using {:?} ({:?}) {}",
91-
executable,
92-
output.status.code().unwrap_or_default(),
93-
stderr
94-
);
95-
}
101+
warn!(
102+
"Failed to get conda info using {:?} ({:?}) {}",
103+
executable,
104+
output.status.code().unwrap_or_default(),
105+
stderr
106+
);
96107
None
97108
}
98109
}
99110
Err(err) => {
100-
// No point logging the message if conda is not installed or a custom conda exe wasn't provided.
101-
if executable.to_string_lossy() != "conda" {
102-
warn!("Failed to execute conda info {:?}", err);
111+
if !is_missing_default_conda(using_default, &err) {
112+
warn!(
113+
"Failed to execute conda info using {:?}: {}",
114+
executable, err
115+
);
103116
}
104117
None
105118
}
106119
}
107120
}
108121
}
122+
123+
fn is_missing_default_conda(using_default: bool, error: &ProcessError) -> bool {
124+
using_default
125+
&& matches!(error, ProcessError::Spawn(source) if source.kind() == io::ErrorKind::NotFound)
126+
}
127+
128+
#[cfg(test)]
129+
mod tests {
130+
use super::*;
131+
use std::ffi::OsStr;
132+
133+
fn captured(success: bool, stdout: &[u8]) -> Output {
134+
#[cfg(unix)]
135+
use std::os::unix::process::ExitStatusExt;
136+
#[cfg(windows)]
137+
use std::os::windows::process::ExitStatusExt;
138+
#[cfg(unix)]
139+
let status = std::process::ExitStatus::from_raw(if success { 0 } else { 23 << 8 });
140+
#[cfg(windows)]
141+
let status = std::process::ExitStatus::from_raw(if success { 0 } else { 23 });
142+
Output {
143+
status,
144+
stdout: stdout.to_vec(),
145+
stderr: b"fixture diagnostics".to_vec(),
146+
}
147+
}
148+
149+
#[test]
150+
fn conda_probe_uses_shared_deadline_and_preserves_metadata() {
151+
let info = CondaInfo::from_with_runner(None, |command, timeout| {
152+
assert_eq!(command.get_program(), OsStr::new("conda"));
153+
assert_eq!(command.get_args().collect::<Vec<_>>(), ["info", "--json"]);
154+
assert_eq!(timeout, Duration::from_secs(15));
155+
Ok(captured(true, br#" {
156+
"envs":["env-one","env-two"],"conda_prefix":"base","root_prefix":"root",
157+
"conda_version":"25.1.0","envs_dirs":["dir"],"envs_path":["alias"],
158+
"config_files":["config"],"rc_path":"rc","user_rc_path":"user","sys_rc_path":"system"
159+
} "#))
160+
}).unwrap();
161+
assert_eq!(info.executable, PathBuf::from("conda"));
162+
assert_eq!(
163+
info.envs,
164+
[PathBuf::from("env-one"), PathBuf::from("env-two")]
165+
);
166+
assert_eq!(info.conda_prefix, Some("base".into()));
167+
assert_eq!(info.root_prefix, Some("root".into()));
168+
assert_eq!(info.conda_version, "25.1.0");
169+
assert_eq!(
170+
info.envs_dirs,
171+
[PathBuf::from("dir"), PathBuf::from("alias")]
172+
);
173+
assert_eq!(info.config_files, [PathBuf::from("config")]);
174+
assert_eq!(info.rc_path, Some("rc".into()));
175+
assert_eq!(info.user_rc_path, Some("user".into()));
176+
assert_eq!(info.sys_rc_path, Some("system".into()));
177+
}
178+
179+
#[test]
180+
fn conda_rejects_failed_invalid_and_incomplete_probes() {
181+
for (success, stdout) in [
182+
(false, b"{}".as_slice()),
183+
(true, b"{".as_slice()),
184+
(true, b"{\"conda_version\":\"\xff\"}".as_slice()),
185+
] {
186+
assert!(
187+
CondaInfo::from_with_runner(Some("custom-conda".into()), |_, _| Ok(captured(
188+
success, stdout
189+
)))
190+
.is_none()
191+
);
192+
}
193+
for error in [
194+
ProcessError::Spawn(io::Error::from(io::ErrorKind::NotFound)),
195+
ProcessError::Io(io::Error::from(io::ErrorKind::BrokenPipe)),
196+
ProcessError::Timeout(Duration::from_secs(15)),
197+
ProcessError::OutputLimit(4 * 1024 * 1024),
198+
ProcessError::IncompleteOutput(captured(true, b"").status),
199+
] {
200+
assert!(CondaInfo::from_with_runner(None, |_, _| Err(error)).is_none());
201+
}
202+
}
203+
204+
#[test]
205+
fn only_missing_default_conda_is_quiet() {
206+
let missing = ProcessError::Spawn(io::Error::from(io::ErrorKind::NotFound));
207+
assert!(is_missing_default_conda(true, &missing));
208+
assert!(!is_missing_default_conda(false, &missing));
209+
for error in [
210+
ProcessError::Spawn(io::Error::from(io::ErrorKind::PermissionDenied)),
211+
ProcessError::Io(io::Error::from(io::ErrorKind::NotFound)),
212+
ProcessError::Timeout(Duration::from_secs(15)),
213+
ProcessError::OutputLimit(1),
214+
] {
215+
assert!(!is_missing_default_conda(true, &error));
216+
}
217+
}
218+
219+
#[test]
220+
fn manager_noisy_output_and_hang_use_real_bounded_capture() {
221+
let directory = tempfile::tempdir().unwrap();
222+
let workspace = directory.path().to_path_buf();
223+
let executable = workspace.join(if cfg!(windows) {
224+
"manager.cmd"
225+
} else {
226+
"manager"
227+
});
228+
let payload = r#"{"conda_version":"25.1.0"}"#;
229+
#[cfg(windows)]
230+
let script = format!(
231+
"@echo off\r\nfor /L %%i in (1,1,128) do @echo {} 1>&2\r\necho {payload}\r\n",
232+
"x".repeat(1024)
233+
);
234+
#[cfg(unix)]
235+
let script = format!(
236+
"#!/bin/sh\nprintf '%s' '{}' >&2\nprintf '%s\\n' '{payload}'\n",
237+
"x".repeat(128 * 1024)
238+
);
239+
std::fs::write(&executable, script).unwrap();
240+
#[cfg(unix)]
241+
{
242+
use std::os::unix::fs::PermissionsExt;
243+
std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(0o755)).unwrap();
244+
}
245+
let info = CondaInfo::from_with_runner(Some(executable.clone()), |command, timeout| {
246+
assert_eq!(timeout, Duration::from_secs(15));
247+
output(command, Duration::from_secs(5))
248+
})
249+
.expect("noisy Conda fixture must still resolve");
250+
assert_eq!(info.conda_version, "25.1.0");
251+
#[cfg(windows)]
252+
let hang = "@echo off\r\n:loop\r\ngoto loop\r\n";
253+
#[cfg(unix)]
254+
let hang = "#!/bin/sh\nwhile :; do :; done\n";
255+
std::fs::write(&executable, hang).unwrap();
256+
let started = std::time::Instant::now();
257+
assert!(CondaInfo::from_with_runner(Some(executable), |command, _| {
258+
let result = output(command, Duration::from_millis(200));
259+
assert!(matches!(result, Err(ProcessError::Timeout(_))));
260+
result
261+
})
262+
.is_none());
263+
assert!(started.elapsed() < Duration::from_secs(3));
264+
}
265+
266+
#[test]
267+
fn configured_conda_name_logs_not_found_while_implicit_default_is_quiet() {
268+
thread_local! {
269+
static WARNINGS: std::cell::Cell<usize> = const { std::cell::Cell::new(0) };
270+
}
271+
struct ProbeLogger;
272+
impl log::Log for ProbeLogger {
273+
fn enabled(&self, metadata: &log::Metadata<'_>) -> bool {
274+
metadata.level() == log::Level::Warn
275+
}
276+
fn log(&self, record: &log::Record<'_>) {
277+
if record.level() == log::Level::Warn && record.target() == "pet_conda::conda_info"
278+
{
279+
WARNINGS.with(|count| count.set(count.get() + 1));
280+
}
281+
}
282+
fn flush(&self) {}
283+
}
284+
log::set_logger(&ProbeLogger).expect("Conda unit tests must have one logger");
285+
log::set_max_level(log::LevelFilter::Warn);
286+
assert!(log::log_enabled!(target: "pet_conda::conda_info", log::Level::Warn));
287+
for (executable, warnings) in [
288+
(None, 0),
289+
(Some("conda".into()), 1),
290+
(Some("custom-conda".into()), 1),
291+
] {
292+
WARNINGS.with(|count| count.set(0));
293+
assert!(CondaInfo::from_with_runner(executable, |_, _| {
294+
Err(ProcessError::Spawn(io::Error::from(
295+
io::ErrorKind::NotFound,
296+
)))
297+
})
298+
.is_none());
299+
log::logger().flush();
300+
WARNINGS.with(|count| assert_eq!(count.get(), warnings));
301+
}
302+
}
303+
}

0 commit comments

Comments
 (0)