Skip to content

Commit 55739c0

Browse files
committed
test(git-remote): make the Windows Job Object mandatory in run_bounded
Resolves jatmn's #192 P2. run_bounded's clean-exit teardown only terminated the job when one had been established; when assign_job returned None it did nothing, so a git-remote-gitlawb helper descendant that outlived the `git fetch` leader while holding the inherited stdout/stderr pipe write-ends stalled the reader joins on the helper's ~300s HTTP timeout, past the bound run_bounded promises. The timeout path's reap_tree (taskkill /T) fallback cannot rescue the clean path: once the leader has cleanly exited it no longer roots the descendant in the parent-PID tree, so taskkill /T cannot reach the orphan. The Job Object is the only reliable Windows teardown, so make it mandatory: assign_job now returns JobHandle and panics per-step if the job cannot be established, and both teardown paths unconditionally terminate it. Nested unnamed jobs with KILL_ON_JOB_CLOSE are standard on the shipped x86_64-pc-windows-msvc / CI runners, so this costs no coverage; it converts a silent ~300s hang into a loud, actionable failure and strengthens the bound (KILL_ON_JOB_CLOSE kills the descendant at latest when JobHandle drops). reap_tree, whose only Windows use was the removed None arm, is narrowed to cfg(all(not(unix), not(windows))). The change is entirely cfg(windows). Verified by cross-compiling the tests for x86_64-pc-windows-gnu (compiles clean, clippy -D warnings clean, no dead_code from the reap_tree narrowing) and by the unchanged Linux git-remote-gitlawb suite (green). The Windows runtime teardown is reasoned-not-run here; CI's Windows job is the runtime gate.
1 parent a1f863f commit 55739c0

1 file changed

Lines changed: 44 additions & 35 deletions

File tree

‎crates/git-remote-gitlawb/tests/real_git_fetch.rs‎

Lines changed: 44 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -333,21 +333,16 @@ fn reap_group(child: &mut std::process::Child) {
333333
let _ = child.wait();
334334
}
335335

336-
/// Fallback tree teardown for the non-unix timeout path (INV-22): the leader-only
337-
/// counterpart used only when the Job Object could not be established at spawn.
338-
///
339-
/// The Job Object below is the correct primitive on Windows (it owns the tree
340-
/// regardless of leader liveness), so this is now a best-effort fallback rather
341-
/// than the main path. `taskkill /T /F` walks the parent-pid tree from the leader
342-
/// and force-kills every member reachable through it, closing the inherited pipe
343-
/// write-ends so the reader joins in `run_bounded` return instead of waiting out
344-
/// the helper's ~300s HTTP timeout. taskkill ships in System32 on every supported
345-
/// Windows and needs no `unsafe`. Its known limit is exactly why the job exists:
346-
/// /T resolves the parent-pid tree at invocation time and cannot reach a
347-
/// descendant once its leader has already exited. The `child.kill()` below is a
348-
/// further leader-only fallback for the taskkill-unavailable case; finally reap
349-
/// the leader so it does not linger, mirroring `reap_group`'s contract.
350-
#[cfg(not(unix))]
336+
/// Best-effort tree teardown for the fallback non-unix, non-windows timeout path
337+
/// (INV-22): the leader-only counterpart to the unix `reap_group`, for exotic targets
338+
/// that are neither unix (which uses `process_group(0)` + `reap_group`) nor windows
339+
/// (which uses the mandatory Job Object below). It attempts `taskkill /T /F` to walk a
340+
/// parent-pid tree if that tool is present, then force-kills and reaps the leader so it
341+
/// does not linger. With no group primitive on these targets it cannot guarantee a
342+
/// descendant is reached; it is the honest best effort where neither real primitive
343+
/// applies. Windows no longer routes here: the Job Object is mandatory there, so this
344+
/// is not compiled on windows and cannot warn as dead code.
345+
#[cfg(all(not(unix), not(windows)))]
351346
fn reap_tree(child: &mut std::process::Child) {
352347
let _ = Command::new("taskkill")
353348
.args(["/T", "/F", "/PID", &child.id().to_string()])
@@ -389,8 +384,14 @@ impl Drop for JobHandle {
389384

390385
/// Create a Job Object, configure `KILL_ON_JOB_CLOSE`, and assign the freshly
391386
/// spawned `child` to it, mirroring the unix `cmd.process_group(0)` at spawn.
392-
/// Returns `None` if any step fails, in which case the caller falls back to the
393-
/// leader-only `reap_tree` tree walk.
387+
///
388+
/// The job is MANDATORY: it is the only reliable Windows teardown for this harness,
389+
/// so any failure to establish it PANICS rather than returning, which would leave
390+
/// `run_bounded` with no way to bound its clean-exit path (taskkill `/T` cannot reach
391+
/// a helper descendant once the `git fetch` leader has exited and no longer roots it
392+
/// in the parent-PID tree). Nested unnamed jobs with `KILL_ON_JOB_CLOSE` are standard
393+
/// on the shipped `x86_64-pc-windows-msvc` / CI runners, so failing loudly here beats
394+
/// silently entering the unbounded ~300s-hang path (jatmn's #192 P2).
394395
///
395396
/// Honest caveat: this assigns the child right after `spawn` rather than via
396397
/// `CREATE_SUSPENDED` + resume (which std cannot do without exposing the main
@@ -401,7 +402,7 @@ impl Drop for JobHandle {
401402
/// the helper is created inside the job. `KILL_ON_JOB_CLOSE` covers any stray that
402403
/// somehow raced ahead.
403404
#[cfg(windows)]
404-
fn assign_job(child: &std::process::Child) -> Option<JobHandle> {
405+
fn assign_job(child: &std::process::Child) -> JobHandle {
405406
use std::os::windows::io::AsRawHandle;
406407
use windows_sys::Win32::Foundation::HANDLE;
407408
use windows_sys::Win32::System::JobObjects::{
@@ -416,7 +417,10 @@ fn assign_job(child: &std::process::Child) -> Option<JobHandle> {
416417
// borrowed.
417418
let job = unsafe { CreateJobObjectW(std::ptr::null(), std::ptr::null()) };
418419
if job.is_null() {
419-
return None;
420+
panic!(
421+
"real_git_fetch harness: CreateJobObjectW failed; the deadlock-bounding \
422+
harness requires a Windows Job Object to tear down the fetch tree"
423+
);
420424
}
421425
// Own the handle now so an early return below still closes it via Drop.
422426
let owner = JobHandle(job);
@@ -437,17 +441,25 @@ fn assign_job(child: &std::process::Child) -> Option<JobHandle> {
437441
)
438442
};
439443
if ok == 0 {
440-
return None; // `owner` drops here -> CloseHandle.
444+
// `owner` drops on unwind -> CloseHandle.
445+
panic!(
446+
"real_git_fetch harness: SetInformationJobObject(KILL_ON_JOB_CLOSE) failed; \
447+
the deadlock-bounding harness requires a Windows Job Object"
448+
);
441449
}
442450

443451
// SAFETY: assigns the child's OS process handle to the job. Both arguments are
444452
// raw handle values and no Rust memory is borrowed; the `Child` owns the
445453
// process handle and outlives this call, so `as_raw_handle` is valid here.
446454
let ok = unsafe { AssignProcessToJobObject(job, child.as_raw_handle() as HANDLE) };
447455
if ok == 0 {
448-
return None;
456+
// `owner` drops on unwind -> CloseHandle.
457+
panic!(
458+
"real_git_fetch harness: AssignProcessToJobObject failed; the deadlock-bounding \
459+
harness requires the git fetch leader inside a Windows Job Object"
460+
);
449461
}
450-
Some(owner)
462+
owner
451463
}
452464

453465
/// Terminate every member of the job (INV-22): the Windows analog of `reap_group`
@@ -489,8 +501,10 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp
489501
// Windows analog of the unix `process_group(0)` above: Windows has no way to
490502
// set a group before spawn, so assign the freshly spawned leader to a Job
491503
// Object now. The job owns the whole fetch tree and is terminated on both
492-
// teardown paths below (INV-22). `None` if the job could not be established, in
493-
// which case teardown falls back to the taskkill tree walk.
504+
// teardown paths below (INV-22). It is mandatory: `assign_job` panics rather
505+
// than returning if the job cannot be established, because without it the
506+
// clean-exit path below cannot be bounded (taskkill `/T` cannot reach an
507+
// orphaned helper once the leader has exited).
494508
#[cfg(windows)]
495509
let job = assign_job(&child);
496510

@@ -522,15 +536,12 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp
522536
// (INV-22, mirrors gitlawb-node/src/git/smart_http.rs).
523537
#[cfg(unix)]
524538
reap_group(&mut child);
525-
// Windows: terminate the whole job (every assigned member); the taskkill
526-
// tree walk stays only as the fallback when the job was not established.
539+
// Windows: terminate the whole job (every assigned member). The job is
540+
// mandatory (`assign_job` panics otherwise), so there is no None fallback.
527541
#[cfg(windows)]
528-
match &job {
529-
Some(job) => {
530-
terminate_job(job);
531-
let _ = child.wait();
532-
}
533-
None => reap_tree(&mut child),
542+
{
543+
terminate_job(&job);
544+
let _ = child.wait();
534545
}
535546
// Any other non-unix target keeps the leader-only tree walk.
536547
#[cfg(all(not(unix), not(windows)))]
@@ -578,9 +589,7 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp
578589
// already-empty job is a harmless no-op (INV-22).
579590
#[cfg(windows)]
580591
if completed {
581-
if let Some(job) = &job {
582-
terminate_job(job);
583-
}
592+
terminate_job(&job);
584593
}
585594

586595
let stdout = out_reader.join().expect("stdout reader");

0 commit comments

Comments
 (0)