Skip to content

Commit c4235a2

Browse files
Bound the PDM site probe through utils::process (#1067) (#1106)
* Start refactor for #1067 Assisted-by: Claude Code:claude-opus-5-5 * Bound the PDM site probe through utils::process On macOS, scan reads PDM's site config by asking PDM's own Python for platformdirs' site path. That probe kept its own deadline: a tokio timeout around a kill_on_drop child with a private 5 s budget, beside the output_within spawn every other crawler probe uses since #886. The two kill policies could drift apart again. The probe now runs through output_within on the blocking pool, with the shared 10 s PROBE_TIMEOUT. A hung PDM interpreter is killed and reaped at that deadline. A probe that answers in time behaves as before: the same argv, private cwd, scrubbed environment, 4 KiB output limit and JSON check. A new guard test fails when production code outside utils::process spawns a kill_on_drop child. vendor/npm_dir.rs stays on its pending list until its git exchange moves too (#1067). Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d52a67b commit c4235a2

2 files changed

Lines changed: 155 additions & 58 deletions

File tree

‎crates/socket-patch-core/src/crawlers/python_crawler/pdm_site.rs‎

Lines changed: 48 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,16 @@
66
//! we never run PDM itself, which would load project and installed plugins.
77
88
use std::path::{Path, PathBuf};
9-
use std::process::Stdio;
9+
use std::process::{Command, Output};
1010
use std::time::Duration;
1111

1212
use tokio::io::AsyncReadExt;
1313

14-
use crate::utils::fs::open_regular_file;
15-
use crate::utils::process::{command_for, resolve_tool_with};
14+
use crate::utils::fs::{open_regular_file, run_blocking};
15+
use crate::utils::process::{command_for, output_within, resolve_tool_with, PROBE_TIMEOUT};
1616

1717
const HEADER_LIMIT: u64 = 8192;
18-
const OUTPUT_LIMIT: u64 = 4096;
19-
const PROBE_TIMEOUT: Duration = Duration::from_secs(5);
18+
const OUTPUT_LIMIT: usize = 4096;
2019
const PROBE: &str =
2120
"import json, platformdirs; print(json.dumps(str(platformdirs.site_config_path('pdm'))))";
2221

@@ -101,30 +100,29 @@ async fn probe_runtime_site_dir(
101100
.prefix("socket-pdm-site-")
102101
.tempdir()
103102
.ok()?;
104-
let child = runtime_probe_command(interpreter, var, cwd.path())
105-
.spawn()
103+
let command = runtime_probe_command(interpreter, var, cwd.path());
104+
// The shared bounded spawn: null stdin, null stderr, killed and reaped
105+
// at the deadline. `cwd` outlives the child.
106+
let output = run_blocking(move || output_within(command, timeout))
107+
.await
106108
.ok()?;
107-
read_runtime_site_dir(child, timeout).await
109+
runtime_site_dir(&output)
108110
}
109111

110112
fn runtime_probe_command(
111113
interpreter: &Path,
112114
var: &impl Fn(&str) -> Option<String>,
113115
cwd: &Path,
114-
) -> tokio::process::Command {
116+
) -> Command {
115117
// Preserve the interpreter's lexical venv path. Canonicalizing bin/python
116118
// could launch its base Python and lose PDM's installed platformdirs.
117-
let mut command = tokio::process::Command::from(command_for(interpreter));
119+
let mut command = command_for(interpreter);
118120
command
119121
// Unlike -I, -E keeps normal user-site pip installs available.
120122
// Do not use -S: Homebrew's normal site initialization affects the
121123
// sys.base_prefix that platformdirs uses for its default site path.
122124
.args(["-E", "-B", "-c", PROBE])
123-
.current_dir(cwd)
124-
.stdin(Stdio::null())
125-
.stdout(Stdio::piped())
126-
.stderr(Stdio::null())
127-
.kill_on_drop(true);
125+
.current_dir(cwd);
128126
// -E alone is insufficient: site.py/Homebrew's sitecustomize read
129127
// PYTHONUSERBASE/PYTHONHOME directly, and macOS startup can honor
130128
// PYTHONEXECUTABLE despite it. Copy only non-Python ambient variables
@@ -148,34 +146,21 @@ fn runtime_probe_command(
148146
command
149147
}
150148

151-
async fn read_runtime_site_dir(
152-
mut child: tokio::process::Child,
153-
timeout: Duration,
154-
) -> Option<PathBuf> {
155-
let stdout = child.stdout.take()?;
156-
tokio::time::timeout(timeout, async {
157-
let mut output = Vec::new();
158-
stdout
159-
.take(OUTPUT_LIMIT + 1)
160-
.read_to_end(&mut output)
161-
.await
162-
.ok()?;
163-
if output.len() > OUTPUT_LIMIT as usize || !child.wait().await.ok()?.success() {
164-
return None;
165-
}
166-
// Exactly one JSON string, not a guessed line in mixed diagnostics.
167-
let path = PathBuf::from(serde_json::from_slice::<String>(&output).ok()?);
168-
path.is_absolute().then_some(path)
169-
})
170-
.await
171-
.ok()
172-
.flatten()
149+
/// A successful probe printed exactly one JSON string holding an absolute
150+
/// path, not a guessed line in mixed diagnostics.
151+
fn runtime_site_dir(output: &Output) -> Option<PathBuf> {
152+
if output.stdout.len() > OUTPUT_LIMIT || !output.status.success() {
153+
return None;
154+
}
155+
let path = PathBuf::from(serde_json::from_slice::<String>(&output.stdout).ok()?);
156+
path.is_absolute().then_some(path)
173157
}
174158

175159
#[cfg(test)]
176160
mod tests {
177161
use super::*;
178162
use std::os::unix::fs::{symlink, PermissionsExt};
163+
use std::process::Stdio;
179164

180165
fn executable(path: &Path, contents: &str) {
181166
std::fs::write(path, contents).unwrap();
@@ -354,28 +339,33 @@ mod tests {
354339
async fn probe_timeout_kills_its_child() {
355340
let tmp = tempfile::tempdir().unwrap();
356341
let python = tmp.path().join("python");
357-
executable(&python, "#!/bin/sh\nexec /bin/sleep 30\n");
358-
let child = runtime_probe_command(&python, &|_: &str| None, tmp.path())
359-
.spawn()
360-
.unwrap();
361-
// Capture the actual OS child identity before awaiting its timeout;
362-
// do not assume a newly spawned shell has written a PID file yet.
363-
let pid = child.id().unwrap() as i32;
364-
let timeout = Duration::from_millis(100);
342+
let pid_file = tmp.path().join("pid");
343+
// `exec` keeps the recorded pid: no shell grandchild holds the pipe.
344+
executable(
345+
&python,
346+
&format!(
347+
"#!/bin/sh\necho $$ > '{}'\nexec /bin/sleep 30\n",
348+
pid_file.display()
349+
),
350+
);
351+
let timeout = Duration::from_millis(500);
365352
let started = std::time::Instant::now();
366-
assert_eq!(read_runtime_site_dir(child, timeout).await, None);
367-
assert!(
368-
started.elapsed() >= timeout,
369-
"the child failed before the deadline"
353+
assert_eq!(
354+
probe_runtime_site_dir(&python, &|_: &str| None, timeout).await,
355+
None
370356
);
371-
tokio::time::timeout(PROBE_TIMEOUT, async {
372-
// Signal zero only checks existence; kill_on_drop must terminate
373-
// and Tokio must reap this exact child, without a shell grandchild.
374-
while unsafe { libc::kill(pid, 0) } == 0 {
375-
tokio::time::sleep(Duration::from_millis(10)).await;
376-
}
377-
})
378-
.await
379-
.expect("timed-out probe was left running");
357+
let elapsed = started.elapsed();
358+
assert!(elapsed >= timeout, "the child failed before the deadline");
359+
assert!(elapsed < PROBE_TIMEOUT, "the probe outlived its budget");
360+
// The child is killed and reaped before the probe returns. A child
361+
// killed before it recorded its pid is gone as well.
362+
if let Ok(pid) = std::fs::read_to_string(&pid_file) {
363+
let pid: i32 = pid.trim().parse().unwrap();
364+
assert_ne!(
365+
unsafe { libc::kill(pid, 0) },
366+
0,
367+
"timed-out probe was left running"
368+
);
369+
}
380370
}
381371
}
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
//! Architecture guard: child-process deadlines live in `utils::process`.
2+
//!
3+
//! `utils::process::output_within` is the one bounded spawn: it nulls stdin
4+
//! and stderr, kills and reaps the child at the deadline, and never waits on
5+
//! a grandchild that still holds the pipe. A production `kill_on_drop` spawn
6+
//! (a `tokio::time::timeout` around a child) is a second deadline policy that
7+
//! drifts from it (#845, #1067).
8+
9+
use std::path::{Path, PathBuf};
10+
11+
/// Production files that still keep a hand-rolled deadline, relative to
12+
/// `src/`. Migrate a file, then drop it here: a stale entry fails too.
13+
const PENDING: &[&str] = &["vendor/npm_dir.rs"];
14+
15+
/// The production part of a source file: everything above its first
16+
/// `#[cfg(test)]` module.
17+
fn production(source: &str) -> &str {
18+
let mut offset = 0;
19+
for line in source.split_inclusive('\n') {
20+
if line.trim() == "#[cfg(test)]" {
21+
let next = source[offset + line.len()..].trim_start();
22+
if next.starts_with("mod ") || next.starts_with("pub(crate) mod ") {
23+
return &source[..offset];
24+
}
25+
}
26+
offset += line.len();
27+
}
28+
source
29+
}
30+
31+
fn rust_files(dir: &Path, out: &mut Vec<PathBuf>) {
32+
for entry in std::fs::read_dir(dir).unwrap() {
33+
let path = entry.unwrap().path();
34+
if path.is_dir() {
35+
rust_files(&path, out);
36+
} else if path.extension().is_some_and(|ext| ext == "rs") {
37+
out.push(path);
38+
}
39+
}
40+
}
41+
42+
/// Files under `src` whose production code spawns a `kill_on_drop` child.
43+
fn hand_rolled_deadlines(src: &Path) -> Vec<String> {
44+
let mut files = Vec::new();
45+
rust_files(src, &mut files);
46+
let mut found: Vec<String> = files
47+
.iter()
48+
.filter(|path| {
49+
let source = std::fs::read_to_string(path).unwrap().replace("\r\n", "\n");
50+
production(&source).contains("kill_on_drop(")
51+
})
52+
.map(|path| {
53+
path.strip_prefix(src)
54+
.unwrap()
55+
.to_string_lossy()
56+
.replace('\\', "/")
57+
})
58+
.filter(|relative| relative != "utils/process.rs")
59+
.collect();
60+
found.sort();
61+
found
62+
}
63+
64+
#[test]
65+
fn child_deadlines_go_through_utils_process() {
66+
let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src");
67+
let found = hand_rolled_deadlines(&src);
68+
let new: Vec<_> = found
69+
.iter()
70+
.filter(|file| !PENDING.contains(&file.as_str()))
71+
.collect();
72+
assert!(
73+
new.is_empty(),
74+
"spawn these children through utils::process::output_within, not a \
75+
local kill_on_drop deadline: {new:?}"
76+
);
77+
let stale: Vec<_> = PENDING
78+
.iter()
79+
.filter(|file| !found.iter().any(|found| found == *file))
80+
.collect();
81+
assert!(
82+
stale.is_empty(),
83+
"these files no longer keep a local deadline; drop them from PENDING: {stale:?}"
84+
);
85+
}
86+
87+
#[test]
88+
fn the_guard_sees_a_planted_site_and_ignores_test_modules() {
89+
let tmp = tempfile::tempdir().unwrap();
90+
let src = tmp.path();
91+
std::fs::create_dir_all(src.join("utils")).unwrap();
92+
std::fs::create_dir_all(src.join("crawlers")).unwrap();
93+
let spawn = "fn probe() { tokio::process::Command::new(\"x\").kill_on_drop(true); }\n";
94+
std::fs::write(src.join("utils/process.rs"), spawn).unwrap();
95+
std::fs::write(src.join("crawlers/planted.rs"), spawn).unwrap();
96+
std::fs::write(
97+
src.join("crawlers/tested.rs"),
98+
format!("fn probe() {{}}\n\n#[cfg(test)]\nmod tests {{\n {spawn}}}\n"),
99+
)
100+
.unwrap();
101+
std::fs::write(
102+
src.join("crawlers/crlf.rs"),
103+
format!("fn a() {{}}\r\n#[cfg(test)]\r\nmod tests {{}}\r\n{spawn}"),
104+
)
105+
.unwrap();
106+
assert_eq!(hand_rolled_deadlines(src), ["crawlers/planted.rs"]);
107+
}

0 commit comments

Comments
 (0)