test(node,git): wire run_git_service teardown end-to-end (#62)

[beardthelion]

Part 3 of #62 (served-git process hardening), and the piece to land first: it locks the teardown invariant the timeout work (part 1) will lean on.

Why

The #61 tests exercise KillGroupOnDrop in isolation. They hand-build the guard and never call run_git_service, so the production wiring is unprotected: deleting command.process_group(0), the guard-arming, or the post-reap disarm() leaves the whole suite green.

What

• A minimal injectable git_bin parameter on the private run_git_service (production callers pass "git"). It exists purely so a fake git can drive the real function end-to-end without mutating the process-global PATH, which keeps the tests parallel-safe.
• Three end-to-end tests that drive run_git_service through that seam and assert the production spawn path actually wires the teardown:
  • dropping the request future mid-flight SIGTERMs the whole group, reaping git and its pack-objects grandchild;
  • a request that runs to completion (both non-zero and zero exit) disarms the guard, leaving a still-live grandchild untouched.

Verification

Every assertion is load-bearing, shown RED against its mutation and green when restored:

• remove process_group(0) or the guard-arming, and the future-drop test fails;
• remove disarm(), and both completion tests fail;
• gate disarm() on failure only, and the success-path test fails while the error-path test passes.

No production behavior change: the wrapper callers pass "git", identical to the previous hardcoded value, and run_git_service has no other callers. cargo fmt and clippy -D warnings are clean.

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of Git service requests, reducing the chance of stray background processes after interrupted or completed operations.
  • Made Git upload and receive actions handle failures more cleanly and consistently.

[coderabbitai]

📝 Walkthrough

Walkthrough

The run_git_service function is refactored to accept an explicit git_bin parameter for constructing its Command, with callers (upload_pack, receive_pack) updated to pass "git". New Unix regression tests validate process-group SIGTERM on drop and kill-guard disarm on completion, using a fake git script and PID tracking helpers.

Changes

Injectable git binary and process-group teardown tests

Layer / File(s)	Summary
run_git_service git binary injection crates/gitlawb-node/src/git/smart_http.rs	run_git_service now takes git_bin: &str to build Command::new(git_bin); upload_pack and receive_pack pass "git" explicitly.
Test helpers for fake git and PID tracking crates/gitlawb-node/src/git/smart_http.rs	Adds a ReapOnPanic drop guard that SIGKILLs recorded PIDs, plus helpers to write an executable fake git script and read leader/grandchild PIDs from a pidfile.
Process-group teardown regression tests crates/gitlawb-node/src/git/smart_http.rs	Adds Unix tokio tests verifying that dropping the future mid-flight SIGTERMs the process group, and that the kill guard disarms correctly on both success and non-zero exit completion.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test
  participant run_git_service
  participant FakeGit
  participant ProcessGroup
  Test->>run_git_service: call with injected git_bin
  run_git_service->>FakeGit: spawn process in new group
  FakeGit->>ProcessGroup: spawn grandchild process
  alt future dropped mid-flight
    Test->>run_git_service: drop future
    run_git_service->>ProcessGroup: SIGTERM entire group
    ProcessGroup-->>Test: grandchild no longer alive
  else completes normally (Ok or Err)
    run_git_service->>run_git_service: wait_with_output, disarm kill guard
    run_git_service-->>Test: grandchild remains alive
  end

Loading

Possibly related issues

• Harden served-git process handling: timeout, cross-env pid cap, wiring test (follow-up to #61) #62: The PR implements the run_git_service wiring test and fake-git injection described in this issue.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change: end-to-end teardown wiring for run_git_service.
Description check	✅ Passed	The description covers the motivation, changes, and verification, though it omits some template sections like kind of change and reviewer checklist.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/served-git-teardown-wiring

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/gitlawb-node/src/git/smart_http.rs (1)

1009-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Optional: fold the two disarm tests into one parameterized helper.

run_git_service_disarms_on_completion_leaving_group_alive and run_git_service_disarms_on_success_leaving_group_alive are near-identical; they differ only in the fake git's exit code (1 vs 0) and the is_err/is_ok assertion. A small helper taking the exit code and an expected-result predicate would remove ~40 duplicated lines while keeping both as distinct #[tokio::test] entry points. Not blocking.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/git/smart_http.rs` around lines 1009 - 1099, The two
`run_git_service_disarms_on_*_leaving_group_alive` tests are duplicated except
for the fake git exit code and the `is_err`/`is_ok` check. Refactor the shared
setup in `run_git_service_disarms_on_completion_leaving_group_alive` and
`run_git_service_disarms_on_success_leaving_group_alive` into a small
parameterized helper that accepts the exit status and an expected-result
predicate, then keep both `#[tokio::test]` functions as thin wrappers so
coverage stays explicit while removing the repeated body.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/gitlawb-node/src/git/smart_http.rs`:
- Around line 1009-1099: The two
`run_git_service_disarms_on_*_leaving_group_alive` tests are duplicated except
for the fake git exit code and the `is_err`/`is_ok` check. Refactor the shared
setup in `run_git_service_disarms_on_completion_leaving_group_alive` and
`run_git_service_disarms_on_success_leaving_group_alive` into a small
parameterized helper that accepts the exit status and an expected-result
predicate, then keep both `#[tokio::test]` functions as thin wrappers so
coverage stays explicit while removing the repeated body.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1e2da2c4-3557-4296-8b9f-5893f7cce5a2

📥 Commits

Reviewing files that changed from the base of the PR and between 563c456 and 7ab4596.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/git/smart_http.rs

[jatmn]

Thanks for the contribution. I do not see any actionable issues from my review.

@kevincodex1 LGTM

[kevincodex1]

LGTM