Skip to content

Check the OpenVMM child's descriptors in the inheritance test - #572

Merged
Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
agents/fix-descriptor-inheritance-test-race
Oct 15, 2026
Merged

Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
agents/fix-descriptor-inheritance-test-race

Conversation

@ppenna

Copy link
Copy Markdown
Contributor

Summary

openvmm_inherits_no_descriptor_beyond_its_standard_streams could fail even though OpenVMM inherits nothing.

  • Before: the test makes a pipe writer inheritable while it spawns a child, then waited up to 2 s for the pipe's reader to reach end-of-file. A plain child that a concurrent test spawned in that window also inherited the writer and kept the pipe open past the timeout. An example is the python3 child of a_process_runs_until_its_last_thread_exits.
  • Now: the test reads the spawned child's own /proc/PID/fd and looks for the planted pipe. Command::spawn returns once the child has executed its program, which closes its close-on-exec copies, so any copy that remains was inherited. The result no longer depends on other processes or on a timeout, and the test takes milliseconds instead of more than 2 s.

Only the test changes. I found this while investigating flaky guest tool tests in #528, which had the same kind of descriptor race. This test was never seen failing on its own, because a long-lived plain child has to start within a window of about a millisecond.

Validation

  • WSL Linux, on 4927f941: every check-nvx-rs and check-aci-edge-sandboxes command passed. That covers formatting, Clippy, tests with all and with default features, the bundled-artifact fixture, agent bundling, rustdoc, Rust 1.89, and the macOS cross-check.
  • Repeated runs: the fixed test passed 200 consecutive runs, 527 ms in total. Its module, which includes the python3 test, passed 100 runs, 10 at a time.
  • Reproduction: a scratch-only test started a bystander plain child while the writer was inheritable. The old check reported a leak after 3.0 s; the new one correctly reported none in 1.2 ms.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 17:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused test-only change removes the cross-test race without identified blocking issues.

0 open findings

What changed in this PR

Makes the Linux descriptor-inheritance test inspect the intended child directly, avoiding false failures caused by concurrent tests.

Changes:

  • Replaces pipe EOF timeouts with /proc/PID/fd inspection.
  • Preserves leak detection checks and child cleanup.
File Description
nvx-rs/​src/​host/​process.rs Updates the test helper and callers to check child descriptors directly.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

openvmm_inherits_no_descriptor_beyond_its_standard_streams makes a pipe
writer inheritable while it spawns a child, then waited up to 2 s for
the reader to reach end-of-file. A plain child that a concurrent test
spawned in that window, such as the python3 child of
a_process_runs_until_its_last_thread_exits, inherited the writer too and
held the pipe open, so the test could fail although OpenVMM inherited
nothing.

Read the child's /proc/PID/fd instead and look for the planted pipe.
Command::spawn returns once the child has executed its program, which
closed its close-on-exec copies, so a copy that remains was inherited.
The result no longer depends on other processes or a timeout, and the
test takes milliseconds instead of more than 2 s.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 15, 2026 18:13
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-descriptor-inheritance-test-race branch from ac63212 to 58c9577 Compare October 15, 2026 18:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused test-only change removes cross-process interference while preserving leak detection and cleanup, with no unresolved findings.

0 open findings

🧠 Review effort: Balanced

@ppenna
Pedro Henrique Penna (ppenna) merged commit 4d48dcd into dev Oct 15, 2026
31 checks passed
@ppenna
Pedro Henrique Penna (ppenna) deleted the agents/fix-descriptor-inheritance-test-race branch October 15, 2026 19:13
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.

2 participants