Repository navigation
nvx-rs: replace the loopback System wrapper with a spawn hook - #607
Merged
Pedro Henrique Penna (ppenna) merged 1 commit intoOct 19, 2026
Merged
Conversation
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>
Contributor
There was a problem hiding this comment.
🟢 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
ENOSYSresult and call counting. - Removes forwarding methods and uses
TestSystemdirectly 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
Pedro Henrique Penna (ppenna)
deleted the
nvx-rs-quality/loopback-spawn-hook
branch
October 19, 2026 17:47
This was referenced Oct 19, 2026
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>
This was referenced Oct 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The managed agent's loopback test,
guest::managed_agent::loopback::tests::control_session_broker_and_real_agent_loop_back, drives the host'sControlSessionthrough a broker shim into the real agent. It needs aSystemthat records mounts and diagnostics as the test doubleTestSystemdoes but really spawns each workload. It got one fromLoopbackSystem, a 60-line wrapper inloopback.rsthat forwarded eleven of the twelveSystemmethods to an innerTestSystemand sentspawntospawn_unprivileged. That made it a third implementation ofSystem, besideRealSystemandTestSystem, and every newSystemmethod needed a hand-written forward there too.This change gives
TestSystema spawn hook instead: an optionalspawnerfunction thatTestSystem::spawnruns when it is set. The loopback test sets it tospawn_unprivilegedand usesTestSystemdirectly, andLoopbackSystemgoes away.Systemis 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 changedprotocol/exec.rs,guest/managed_agent/exec.rs, andhost/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 todevchanges. Innvx-rsandaci_edge_sandboxes, those pull requests are #585, #596, #597, #598, and #606, which change most ofhost/,protocol/features.rs,protocol/mod.rs,protocol/tests.rs,testing/,guest/reseed.rs,guest/time/mod.rs, andguest/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:TestSystemgainsspawner: Option<fn(SpawnRequest<'_>) -> Result<ProcessId, Errno>>, which isNoneunless a test sets it.TestSystem::spawncounts the call inspawn_callsas before, then returns the spawner's result, orENOSYSas before when there is none.loopback.rs: deletesLoopbackSystemand itsimpl System. The test builds itsTestSystemwithspawner: Some(spawn_unprivileged)and readsmountsanddiagnosticsfrom it directly. The imports that only the wrapper used (CStr,Mode,MountFlags, andSystem) go too.TestSystem'sSystemimplementation now serves the loopback test as well as thecgroup,dispatch,exec,helper, andmappingstests, which already used it.Behavior
TestSystembuilds only undercfg(test), and theloopbackmodule only undercfg(all(test, feature = "host")). TheSystemtrait,RealSystem, the agent, and the guest binaries are unchanged.spawnerunset, so itsspawnstill counts the call and fails withENOSYS.Systemhas no default methods, soLoopbackSystemforwarded every operation exceptspawn, unchanged, to itsTestSystem. The loopback test now calls those sameTestSystemmethods directly and spawns through the samespawn_unprivileged. The one difference is that its spawns now count inspawn_calls, which the test does not check.dispatch::tests::containment_failures_refuse_consecutive_execs_until_verified_emptycovers an unset one: it countsspawn_callsand expects thelaunch-failedreply that theENOSYSspawn 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::setupanddispatch::tests::direct_systemare identical;mappings::tests::payloadanddispatch::tests::maps_payloadare identical;mappings::tests::setupbuilds the same host-export tree asdispatch::tests::mapping_system.Two more
AgentConfigfixtures join them.dispatch'sHarness::with_systemand anexectest build the sameAgentConfig, andlaunch::tests::configdiffers from it only in its hostname and GID. Shared constructors next toTestSysteminseam.rscould serve all of these.In
guest/sys/spawn.rs,SpawnDescriptors::managedand the test-onlySpawnDescriptors::plain, whichspawn_unprivilegeduses, differ only ininherit_config. One private constructor could serve both.In
guest/time/commands.rs,cmd_sampleandcmd_generation_idreport a failure to take the port-window lock with the same four lines.In
tests/guest_tools.rs,device_iorepeats the spawn and output collection ofrun, because it keeps theExitStatusfor its message. A helper that returns theOutputcould serve both.These follow-ups of earlier runs also remain:
fail_fatalcalls inguest/time/daemon.rs, which first need a test seam;Instance::runtimeandInstance::launchinhost/instance.rs;guest/sys/socket.rs;run_fileandrun_networkinguest/device_io.rs;reseed.rs,time/mod.rs, andtime_probe/mod.rsland.I found no suspected bug in the code that this change touches.
Validation
BASE:
689e2d164257f0f5dc172057b121dd272e702582, which wasorigin/devwhen 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/devhad advanced only by another performance-baseline update, which does not changenvx-rs, and no open pull request todevchanged either file.TREE:
066a3b3b7b65b34b34dd5609afd440793a19c98f, the tree of this commit. Every host applied the patch to BASE and checked thatgit write-treeequals 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, sodownloadtook 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;build-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-256f6ef08ac961769f02ae0c3d4722c896a55eaa86ab40fd7694947ff073a62d5ce), 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:
-D warnings:x86_64-unknown-linux-gnu, both with all targets and features and for the library without default features;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-onlyguestmodule;-D warnings;aarch64-apple-darwin) checks, for bothnvx-rsandaci_edge_sandboxes.test-aci-edge-sandboxestest-microvm, default suitecheck-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 locallyOn every host, the suite's time-ABI evidence line counted no boot, capture, or restore check over its CPU-time budget.
Left to CI:
test-openvmm-unitandtest-openvmm;smp-lapic,managed-exec-config, andsandbox-lifecycle;