Skip to content

fix(minvmd): replace raw-PID gvproxy signalling with pidfd - #555

Merged
norrietaylor merged 2 commits into
mainfrom
sdd/550-gvproxy-pidfd-3343ddf2bf060369
Jun 24, 2026
Merged

fix(minvmd): replace raw-PID gvproxy signalling with pidfd#555
norrietaylor merged 2 commits into
mainfrom
sdd/550-gvproxy-pidfd-3343ddf2bf060369

Conversation

@gominimal-aw-bot

@gominimal-aw-bot gominimal-aw-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

[sdd-fastpath: tracking=550 tier=sonnet]

Fixes the independent-crash recycled-PID window in GvproxySwitch (#550).

What changed

Replaces raw-PID kill(2) on all three teardown paths with pidfd_send_signal(2) via libc::syscall(SYS_pidfd_send_signal, ...):

  • GvproxySwitch::stop()
  • GvproxySwitch::Drop
  • PtaskAttachment::Drop

pidfd_open(2) is called immediately after the child PID is extracted in supervise(), before the supervision task is spawned. The Arc<OwnedFd> is shared with every PtaskAttachment so all teardown paths use the same pidfd. A pidfd is bound to the exact process instance — pidfd_send_signal returns ESRCH after the process exits, never resolving to a recycled PID.

signal_child (raw kill(2)) is retained under #[cfg(not(target_os = "linux"))] for macOS development builds.

Proof artifacts

Test — cargo test -p minvmd net::tests::pidfd_signal_to_reaped_child_returns_esrch:

test net::tests::pidfd_signal_to_reaped_child_returns_esrch ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 71 filtered out; finished in 0.00s

File — grep -q 'pidfd' crates/minvmd/src/net.rs:
Returns exit code 0 (pidfd is present in net.rs).

All 19 net tests (including existing teardown tests):

test net::tests::drop_sigkills_supervised_child_without_blocking ... ok
test net::tests::last_ptask_detach_terminates_switch ... ok
test net::tests::pidfd_signal_to_reaped_child_returns_esrch ... ok
test net::tests::stop_terminates_supervised_child ... ok
test net::tests::unexpected_exit_fires_notify ... ok
test result: ok. 19 passed; 0 failed; 0 ignored; 0 measured; 53 filtered out; finished in 0.02s

Merging this pull request advances the tracking issue from sdd:in-progress to sdd:done; a human does the final close.

Generated by sdd-execute (sonnet tier) ·

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved Linux process supervision and teardown reliability by using safer, recycle-resistant signaling to ensure supervised services terminate and clean up correctly.
  • Tests

    • Added Linux-only coverage to confirm correct pidfd signaling behavior, including expected failure after a process has been reaped.

Use pidfd_open(2) to obtain a process-identity file descriptor for the
gvproxy child at spawn time, and route all teardown signals through
pidfd_send_signal(2) via libc::syscall(SYS_pidfd_send_signal, …).

A pidfd is bound to the exact process instance, not the numeric PID.
After the process exits, pidfd_send_signal returns ESRCH rather than
silently delivering a signal to an unrelated process that recycled the
same PID — structurally closing the crash-then-recycle window documented
in the PtaskAttachment::Drop comment.

Changes:
- GvproxySwitch::supervise opens a pidfd immediately after extracting
  the child PID, before the supervision task is spawned (Arc<OwnedFd>
  shared with PtaskAttachment so all three teardown paths use it).
- signal_via_pidfd replaces signal_child on Linux for all three
  teardown paths: stop(), GvproxySwitch::Drop, PtaskAttachment::Drop.
- signal_child is retained under #[cfg(not(target_os = "linux"))]
  for macOS development builds.
- New test pidfd_signal_to_reaped_child_returns_esrch: spawns a
  short-lived child, opens its pidfd, reaps the child, then asserts
  pidfd_send_signal returns ESRCH rather than hitting a recycled PID.

Refs: #550
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d6d4c7e2-252b-4b59-a712-c6aba59cbdd6

📥 Commits

Reviewing files that changed from the base of the PR and between ef5bb03 and cfae4e6.

📒 Files selected for processing (1)
  • crates/minvmd/src/net.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/minvmd/src/net.rs

📝 Walkthrough

Walkthrough

Adds Linux pidfd-based signalling to GvproxySwitch and PtaskAttachment in crates/minvmd/src/net.rs. Both structs gain a Linux-only pidfd: Arc<OwnedFd> field. supervise() now opens the pidfd immediately after spawning the child and returns io::Result. All teardown (stop, Drop) and last-detach signal paths use pidfd_send_signal on Linux; a new signal_via_pidfd helper centralises that call. A Linux-only test confirms ESRCH is returned after the child is reaped.

Changes

Pidfd-based recycle-safe signalling for gvproxy

Layer / File(s) Summary
Struct fields and imports for pidfd
crates/minvmd/src/net.rs
Adds Linux-only AsRawFd, FromRawFd, OwnedFd imports and a pidfd: Arc<OwnedFd> field to both GvproxySwitch and PtaskAttachment with updated doc comments.
supervise() pidfd opening and io::Result return
crates/minvmd/src/net.rs
Reworks GvproxySwitch::supervise to return io::Result, calls pidfd_open on the freshly spawned child (Linux only), propagates the Arc<OwnedFd> clone into PtaskAttachment at both attach paths, and updates the spawn call site to propagate errors via ?.
signal_via_pidfd helper and teardown signalling
crates/minvmd/src/net.rs
Introduces Linux-only signal_via_pidfd wrapping pidfd_send_signal with ESRCH suppression. Updates GvproxySwitch::stop, Drop for GvproxySwitch, and last-detach in Drop for PtaskAttachment to use pidfd on Linux instead of numeric-pid kill.
pidfd test and formatting adjustments
crates/minvmd/src/net.rs
Adds pidfd_signal_to_reaped_child_returns_esrch (Linux-only) asserting ESRCH after child reap; applies minor formatting fixes to supervise_sleep and unexpected_exit_fires_notify tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • gominimal/minimal#522: Modifies the same GvproxySwitch supervision and teardown path (supervise, stop, Drop) that this PR extends with pidfd signalling.
  • gominimal/minimal#552: Directly overlaps — switches GvproxySwitch/PtaskAttachment teardown from kill(pid, …) to pidfd_open + pidfd_send_signal, including the ESRCH test.

Suggested reviewers

  • norrietaylor

Poem

🐇 A pidfd, a fd, bound tight to one pid,
No recycled ghost can fool what we did.
SIGTERM through the kernel, ESRCH on the dead,
The rabbit hops safely, no races ahead.
Arc'd and shared, the fd lives on—
Teardown is clean, the race window gone! 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: replacing raw PID signalling with pidfd for gvproxy process supervision, which aligns with the primary objective of fixing the race condition.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@crates/minvmd/src/net.rs`:
- Around line 405-426: When the `pidfd_open` syscall fails (in the `if raw < 0`
check), the function returns an error before the supervision task is spawned,
causing the `child` process to be dropped without being killed due to the
default `kill_on_drop = false` behavior. Before returning the error on the
failed `pidfd_open` call, call the appropriate kill method on the `child`
process to ensure it is terminated and prevent the process leak. This ensures
the child does not remain orphaned when `pidfd_open` fails.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b5cedf65-c347-41c8-8e24-cb7ec04fc9f2

📥 Commits

Reviewing files that changed from the base of the PR and between 1db2cbf and ef5bb03.

📒 Files selected for processing (1)
  • crates/minvmd/src/net.rs

Comment thread crates/minvmd/src/net.rs
@github-actions

Copy link
Copy Markdown

Revise claim for head ef5bb03.

@github-actions

Copy link
Copy Markdown

Auto-revise 1 of 3.

@gominimal-aw-bot

This comment has been minimized.

@gominimal-aw-bot gominimal-aw-bot Bot mentioned this pull request Jun 24, 2026
When pidfd_open(2) fails, the owned Child was returned without being
killed. Because tokio::process::Child defaults to kill_on_drop = false,
the gvproxy process would be orphaned. Call start_kill() before
propagating the error so the child is always terminated on this path.
@gominimal-aw-bot

Copy link
Copy Markdown
Contributor Author

Commit pushed: cfae4e6

Generated by sdd-execute (sonnet tier)

@gominimal-aw-bot

Copy link
Copy Markdown
Contributor Author

sdd-validate findings

Boundary: Implementation
Feature: #550 (fast-path — sdd-execute advances lifecycle on merge)
Head: cfae4e6 (re-validation after push synchronize)


Gate 1 — Proof artifacts re-executed and passing

Artifact 1 — File: grep -q 'pidfd' crates/minvmd/src/net.rs

  • Executed locally. pidfd found 43 times in crates/minvmd/src/net.rs.
  • Result: passes ✓

Artifact 2 — Test: cargo test -p minvmd net::tests::pidfd_signal_to_reaped_child_returns_esrch

  • Infrastructure limit: index.crates.io blocked by network firewall (CONNECT tunnel 403). cargo cannot fetch dependencies, so the command cannot run in this container.
  • Covering consumer check: the ci-success required status check on main includes a test job that runs cargo nextest run --workspace, which exercises this test.
  • Result: deferred to consumer CI (Info — ci-success / test job covers this proof)

Gate 2 — Changed files within task scope

Execution plan ([sdd-spec:fastpath-plan] on #550) declares files in scope: crates/minvmd/src/net.rs.
The PR changes exactly one file: crates/minvmd/src/net.rs.

  • Result: within scope ✓

Gate 3 — No real credentials in the diff

The diff contains Rust code using Linux pidfd syscalls and Arc<OwnedFd> plumbing. No secrets, tokens, or keys.

  • Result: clean ✓

Verdict

Implementation boundary passes clean. No Blockers, no Warnings.

This is a fast-path feature: lifecycle advance (sdd:in-progresssdd:done) is sdd-execute's responsibility on implementation PR merge (ADR 0012).

Generated by sdd-validate (run 28072944784)

Generated by sdd-validate for issue #555 ·

@norrietaylor
norrietaylor enabled auto-merge (squash) June 24, 2026 04:17
@norrietaylor
norrietaylor merged commit a92c8fb into main Jun 24, 2026
36 checks passed
@norrietaylor
norrietaylor deleted the sdd/550-gvproxy-pidfd-3343ddf2bf060369 branch June 24, 2026 04:17
norrietaylor added a commit that referenced this pull request Jun 24, 2026
…sh proof

Restore main's crates/minvmd/src/net.rs: this branch had reverted #555's
pidfd-based gvproxy signalling (a stale-base content-revert unrelated to
the WireGuard mesh task), re-introducing the PID-recycle bug. WireGuard
lives in minimald; minvmd net.rs is out of scope here.

Wire the UC7 two-namespace mesh proof into ci-netns.yml: it was covered
by no gate (mesh_uc7.rs is networking-wg-gated, the netns name filter did
not match remote_ptask_packet_crosses_the_mesh_tunnel, and the test read
MINIMAL_NETNS_TESTS while the lane sets MINIMALD_NETNS_TEST). Add a
dedicated --features networking-wg --test mesh_uc7 step and fix the env
var name in the test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
norrietaylor added a commit that referenced this pull request Jun 24, 2026
…sh proof

Restore main's crates/minvmd/src/net.rs: this branch had reverted #555's
pidfd-based gvproxy signalling (a stale-base content-revert unrelated to
the WireGuard mesh task), re-introducing the PID-recycle bug. WireGuard
lives in minimald; minvmd net.rs is out of scope here.

Wire the UC7 two-namespace mesh proof into ci-netns.yml: it was covered
by no gate (mesh_uc7.rs is networking-wg-gated, the netns name filter did
not match remote_ptask_packet_crosses_the_mesh_tunnel, and the test read
MINIMAL_NETNS_TESTS while the lane sets MINIMALD_NETNS_TEST). Add a
dedicated --features networking-wg --test mesh_uc7 step and fix the env
var name in the test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
norrietaylor added a commit that referenced this pull request Jun 24, 2026
… advertisement, and minimal mesh CLI (#561)

* feat(minimald,minimal2): boringtun WireGuard mesh peer and mesh CLI

* ci: gate the networking-wg WireGuard mesh proofs

The WireGuard mesh peer is behind the non-default networking-wg feature,
so the workspace test job never compiled or ran its proof artifacts
(two_meshes_handshake_and_relay_a_packet, rpc get_mesh_status). Add an
explicit step so the mesh proofs run in CI (sdd-validate Gate 1).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS

* fix(minimald): restore pidfd net.rs revert + wire up the UC7 netns mesh proof

Restore main's crates/minvmd/src/net.rs: this branch had reverted #555's
pidfd-based gvproxy signalling (a stale-base content-revert unrelated to
the WireGuard mesh task), re-introducing the PID-recycle bug. WireGuard
lives in minimald; minvmd net.rs is out of scope here.

Wire the UC7 two-namespace mesh proof into ci-netns.yml: it was covered
by no gate (mesh_uc7.rs is networking-wg-gated, the netns name filter did
not match remote_ptask_packet_crosses_the_mesh_tunnel, and the test read
MINIMAL_NETNS_TESTS while the lane sets MINIMALD_NETNS_TEST). Add a
dedicated --features networking-wg --test mesh_uc7 step and fix the env
var name in the test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS

* ci(netns): run the UC7 mesh proof binary as root

mesh_uc7 binds sockets inside the namespaces via in-process setns, so the
test binary itself needs root (the sudo-per-command model the UC6 netns
tests use does not cover an in-process setns). Build unprivileged, then
run the test binary under the netns runner's passwordless sudo, fixing the
'mkdir /run/netns: Permission denied' / 'ip netns add' failures.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS

* fix(minimald,minimal2): address mesh review threads

- mesh join: validate host:port at entry before persisting the enrolment
  so a typo never lands a bad address on disk (CR/bot R4.3 input check).
- wg route_inbound: probe every candidate peer (exact-endpoint match
  first, then each endpoint-less peer) until one authenticates, instead
  of stopping at the first endpoint-less peer — handles roaming source
  addresses and multiple endpoint-less peers. Still exactly one
  decapsulate for the owning peer.
- wg mesh_status: treat a finished pump as unconfigured (MeshHandle::
  is_alive) so GetMeshStatus never serves frozen, stale peer state after
  the pump exits on a socket error.
- wg loopback test: pre-bind ephemeral sockets via start_with_socket
  instead of hard-coding 51820/51821, removing CI port-collision flakes.
- mesh_uc7: RAII Drop guard tears down namespaces on every exit path
  (success, panic, timeout); fix stale env-var name in the doc comment.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(minvmd): always SIGKILL gvproxy on stop() timeout

When a PtaskAttachment drop already claimed teardown (SIGTERM sent) and
gvproxy ignores SIGTERM, stop() would hit its grace timeout but skip
SIGKILL under the `!already_claimed` guard, then block forever on
`supervisor.await` and hang daemon shutdown. Escalate to SIGKILL on
timeout unconditionally; on Linux the fd-based signal targets the exact
process instance (ESRCH after exit is benign), so it never lands on a
recycled PID.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(minimal2): reject port 0 in mesh-join enrolment address

port.parse::<u16>() accepts 0, but a WireGuard endpoint on port zero is
unusable; was still written as a successful enrolment. Require non-zero.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS

---------

Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com>
Co-authored-by: Norrie Taylor <norrie@minimal.dev>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant