Skip to content

nvx-rs: replace the loopback System wrapper with a spawn hook - #607

Merged
Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/loopback-spawn-hook
Oct 19, 2026
Merged

Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/loopback-spawn-hook

Conversation

@ppenna

Copy link
Copy Markdown
Contributor

Summary

The managed agent's loopback test, guest::managed_agent::loopback::tests::control_session_broker_and_real_agent_loop_back, drives the host's ControlSession through a broker shim into the real agent. It needs a System that records mounts and diagnostics as the test double TestSystem does but really spawns each workload. It got one from LoopbackSystem, a 60-line wrapper in loopback.rs that forwarded eleven of the twelve System methods to an inner TestSystem and sent spawn to spawn_unprivileged. That made it a third implementation of System, beside RealSystem and TestSystem, and every new System method needed a hand-written forward there too.

This change gives TestSystem a spawn hook instead: an optional spawner function that TestSystem::spawn runs when it is set. The loopback test sets it to spawn_unprivileged and uses TestSystem directly, and LoopbackSystem goes away. System is left with two implementations: the production one and the test double.

This is the fourth nvx-rs-quality/ run. #603 and #605 merged and #606 is open; all three had approving Copilot reviews and no feedback to follow. They changed protocol/exec.rs, guest/managed_agent/exec.rs, and host/filesystem.rs. This run surveyed the rest of the crate, by reading and with a throwaway script that reports every repeated window of six or more normalized lines, also with names and literals masked. It skipped every file that an open pull request to dev changes. In nvx-rs and aci_edge_sandboxes, those pull requests are #585, #596, #597, #598, and #606, which change most of host/, protocol/features.rs, protocol/mod.rs, protocol/tests.rs, testing/, guest/reseed.rs, guest/time/mod.rs, and guest/time_probe/mod.rs, among others. It picked this follow-up of #605. Among the candidates in files that no open pull request or recent run touches, it removes the most code per changed line.

Changes

Two files in nvx-rs/src/guest/managed_agent/ change, with 16 insertions and 74 deletions. No public item, production code path, message, fixture, or other file changes.

  • seam.rs: TestSystem gains spawner: Option<fn(SpawnRequest<'_>) -> Result<ProcessId, Errno>>, which is None unless a test sets it. TestSystem::spawn counts the call in spawn_calls as before, then returns the spawner's result, or ENOSYS as before when there is none.
  • loopback.rs: deletes LoopbackSystem and its impl System. The test builds its TestSystem with spawner: Some(spawn_unprivileged) and reads mounts and diagnostics from it directly. The imports that only the wrapper used (CStr, Mode, MountFlags, and System) go too.

TestSystem's System implementation now serves the loopback test as well as the cgroup, dispatch, exec, helper, and mappings tests, which already used it.

Behavior

  • Only test code changes. TestSystem builds only under cfg(test), and the loopback module only under cfg(all(test, feature = "host")). The System trait, RealSystem, the agent, and the guest binaries are unchanged.
  • Every test except the loopback test leaves spawner unset, so its spawn still counts the call and fails with ENOSYS.
  • System has no default methods, so LoopbackSystem forwarded every operation except spawn, unchanged, to its TestSystem. The loopback test now calls those same TestSystem methods directly and spawns through the same spawn_unprivileged. The one difference is that its spawns now count in spawn_calls, which the test does not check.
  • Existing tests cover both paths of the hook, so this change adds none:
    • the loopback test covers a set spawner;
    • dispatch::tests::containment_failures_refuse_consecutive_execs_until_verified_empty covers an unset one: it counts spawn_calls and expects the launch-failed reply that the ENOSYS spawn causes.

Follow-ups

The survey turned these up, and this change leaves them alone.

  • The managed-agent test fixtures that nvx-rs: track open workload output pipes by their Option alone #605 listed remain:

    • cgroup::tests::setup and dispatch::tests::direct_system are identical;
    • mappings::tests::payload and dispatch::tests::maps_payload are identical;
    • mappings::tests::setup builds the same host-export tree as dispatch::tests::mapping_system.

    Two more AgentConfig fixtures join them. dispatch's Harness::with_system and an exec test build the same AgentConfig, and launch::tests::config differs from it only in its hostname and GID. Shared constructors next to TestSystem in seam.rs could serve all of these.

  • In guest/sys/spawn.rs, SpawnDescriptors::managed and the test-only SpawnDescriptors::plain, which spawn_unprivileged uses, differ only in inherit_config. One private constructor could serve both.

  • In guest/time/commands.rs, cmd_sample and cmd_generation_id report a failure to take the port-window lock with the same four lines.

  • In tests/guest_tools.rs, device_io repeats the spawn and output collection of run, because it keeps the ExitStatus for its message. A helper that returns the Output could serve both.

  • These follow-ups of earlier runs also remain:

    • the six fail_fatal calls in guest/time/daemon.rs, which first need a test seam;
    • Instance::runtime and Instance::launch in host/instance.rs;
    • the repeated datagram socket creation in guest/sys/socket.rs;
    • run_file and run_network in guest/device_io.rs;
    • the program tests' argument-vector helpers, which can wait until the open pull requests that change reseed.rs, time/mod.rs, and time_probe/mod.rs land.

I found no suspected bug in the code that this change touches.

Validation

  • BASE: 689e2d164257f0f5dc172057b121dd272e702582, which was origin/dev when the run started: a performance-baseline update after the merge of nvx-rs: track open workload output pipes by their Option alone #605. Before this pull request was opened, origin/dev had advanced only by another performance-baseline update, which does not change nvx-rs, and no open pull request to dev changed either file.

  • TREE: 066a3b3b7b65b34b34dd5609afd440793a19c98f, the tree of this commit. Every host applied the patch to BASE and checked that git write-tree equals TREE.

  • Release on every host: v0.1.0-dev.f4b4b8d3f884. BASE and its parent, the merge of nvx-rs: track open workload output pipes by their Option alone #605, have no release, so download took the one from two first-parent commits earlier, the merge of nvx-rs: decode the EXEC header once for both payload decoders #603. Its OpenVMM, 5ab8a84621524f1777b2cb5e65a70dab42c8b453, is the pinned revision.

  • Guest artifacts were rebuilt from TREE on prometheus30, with these steps:

    • build-guest-binaries --output;
    • test-guest-binaries --binaries, where 34 tests passed;
    • the Alpine build-initramfs with NVX_GUEST_BINARIES.

    initramfs.cpio.gz (SHA-256 f6ef08ac961769f02ae0c3d4722c896a55eaa86ab40fd7694947ff073a62d5ce), its packages manifest, and its provenance went to the other two hosts, where the SHA-256 digests matched.

  • Local checks on Windows with Rust 1.95.0:

    • rustfmt;
    • Clippy with -D warnings:
      • for Windows and for x86_64-unknown-linux-gnu, both with all targets and features and for the library without default features;
      • for the guest binaries' configuration: x86_64-unknown-linux-musl, --no-default-features --features guest, and all targets;
    • cargo test --all-features, with 261 library tests, 6 fake-OpenVMM tests, and 1 doctest. On Windows, this does not build the Linux-only guest module;
    • rustdoc with -D warnings;
    • the steps that no host could run, because the hosts lack the toolchain or target: the Windows MSRV (1.89) checks and the macOS (aarch64-apple-darwin) checks, for both nvx-rs and aci_edge_sandboxes.
Host profile Backend Host type check-nvx-rs and check-aci-edge-sandboxes without a hypervisor Guest artifacts test-aci-edge-sandboxes test-microvm, default suite
prometheus32 kvm baremetal Pass. Every Linux step passed, including MSRV 1.89, check-guest-binaries (277 + 34 musl tests), artifact bundling, and agent bundling. The 492 library tests with all features include the loopback test, which passed. The macOS checks ran locally Copied in, digests match Pass (18/18) Pass (41 scenario runs)
prometheus30 mshv baremetal None; prometheus32 and prometheus28 ran the gates Built and tested here Pass (18/18) Pass (41 scenario runs)
prometheus28 whp baremetal Pass. Every Windows step passed, including artifact bundling and agent bundling. The MSRV checks ran locally Copied in, digests match Pass (18/18) Pass (41 scenario runs)

On every host, the suite's time-ABI evidence line counted no boot, capture, or restore check over its CPU-time budget.

Left to CI:

  • the OpenVMM unit and VMM suites, test-openvmm-unit and test-openvmm;
  • the microVM scenarios that run only when named: smp-lapic, managed-exec-config, and sandbox-lifecycle;
  • the Ubuntu and Azure Linux guests, and guest determinism;
  • the benchmarks;
  • the virtual-machine runners;
  • check-quality and validate-nvx, whose inputs this change does not touch.

The managed agent's loopback test needs a System that records mounts
and diagnostics as TestSystem does but really spawns each workload. It
got one from LoopbackSystem, a wrapper that forwarded eleven of the
twelve System methods to an inner TestSystem and sent spawn to
spawn_unprivileged. That made it a third implementation of System,
beside RealSystem and TestSystem, and every new System method needed a
hand-written forward there too.

TestSystem now has a spawner field: a function that spawns each
workload when set. Its spawn still counts the call, then runs the
spawner, or fails with ENOSYS without one, as it always did. The
loopback test sets the spawner to spawn_unprivileged and uses
TestSystem directly, so LoopbackSystem, its 60 lines of forwarding,
and the imports that only it used are gone.

Behavior is unchanged. The change is confined to test code:
TestSystem and the loopback module build only under cfg(test), and
RealSystem, the agent, and the guest binaries are untouched. Every
other test leaves the spawner unset and keeps the ENOSYS spawn. The
loopback test spawns through the same spawn_unprivileged and reaches
the same TestSystem methods for every other operation; it now also
counts its spawns in spawn_calls, which it does not check.

The loopback test covers the spawner, and the dispatch tests, which
count spawn_calls and expect the launch failure that ENOSYS causes,
cover its absence.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 19, 2026 16:10

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 localized test-only refactor leaves production behavior unchanged, with existing tests covering both hook paths and no unresolved findings.

No open issues 🎉

What changed in this PR

Simplifies managed-agent test infrastructure by replacing the loopback-specific System wrapper with a configurable spawn hook on TestSystem.

Changes:

  • Adds an optional spawner while preserving the default ENOSYS result and call counting.
  • Removes forwarding methods and uses TestSystem directly in the loopback test.
File Description
nvx-rs/​src/​guest/​managed_agent/​seam.rs Adds the optional test-only spawn hook.
nvx-rs/​src/​guest/​managed_agent/​loopback.rs Replaces LoopbackSystem with configured TestSystem.

Review effort: Balanced

@ppenna
Pedro Henrique Penna (ppenna) merged commit 4f5e255 into dev Oct 19, 2026
33 checks passed
@ppenna
Pedro Henrique Penna (ppenna) deleted the nvx-rs-quality/loopback-spawn-hook branch October 19, 2026 17:47
Jay (jay23606) pushed a commit to jay23606/nvx that referenced this pull request Oct 19, 2026
A failure of restore-time repair is a fatal time-ABI event: it has the
repair source and the restore phase, and it stops the VM with the
repair power-off status unless the tools run report-only.
commands::repair_failed passed those three to Runtime::fail_fatal, and
daemon.rs spelled them out again at six sites, each with
G_REPAIR_SUPPRESSION. restore_work also inlined the body of
release_suppression. No unit test reached any of them, because
Runtime::emit_event opened /dev/kmsg and /dev/console itself, so
microsoft#605, microsoft#606, and microsoft#607 left this refactor until a test seam existed.

Runtime::repair_failure, beside Runtime::conformance_failure, now
passes the repair status, source, and phase. repair_failed and the six
daemon sites call it, and restore_work calls release_suppression. Two
new PrivilegedOps methods, write_kernel_log and write_console, are the
seam. RealOps runs the code with which emit_event wrote an event's
kernel log record and console line, and the FakeOps of the unit tests
records their bytes, as it already records port writes and power-off
requests.

Behavior is unchanged. Every repair failure passes the status, code,
source, phase, and detail that it passed before. An event writes the
same bytes, with the same open flags, to the kernel log, port B, and
the console, in that order, and reads the asynchronous-output flag at
the same points. A new unit test checks a repair failure's event line
at all three destinations and its power-off status, with and without
report-only mode, in the glibc and musl test runs. The microVM restore
scenarios run restore_work, and so release_suppression, in guests on
KVM, MSHV, and WHP.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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.

2 participants