Repository navigation
nvx-rs: check the guest agent's replies through shared methods - #615
Merged
Pedro Henrique Penna (ppenna) merged 1 commit intoOct 20, 2026
Merged
Conversation
ControlSession, the host side of a sandbox's control session, checked the guest agent's replies with copies of the same code. ping and stop differed only in their request and reply kinds and their message: each sent an empty request, read the next application frame, and failed unless it was an empty reply of the expected kind, with status 0, to that request. features, map_host_paths, and next_exec_event each read a frame and refused one that answered another request with the same message, and map_host_paths and next_exec_event truncated the error category of a refusal to 64 characters the same way. Three private helpers now hold these rules once: acknowledged, the exchange of ping and stop; read_reply, the read that refuses a reply to another request; and error_category, the truncation. Each check lives in one place, so the requests cannot drift apart in how they validate their replies, and a new request can reuse them. Behavior is unchanged. Every message is the same, including "the guest agent did not acknowledge readiness" and "... stop", which acknowledged now formats from what it acknowledges; each method makes the same checks in the same order on the same frames; and the public API is untouched. The existing session tests, the managed agent's loopback tests, the fake-OpenVMM tests of nvx-rs and aci_edge_sandboxes, and the hypervisor suites drive every changed path. Two new unit tests pin what no test did: the requests that ping and stop send and each refusal of their acknowledgement, and the mismatched-request-ID message of features, map_host_paths, and next_exec_event. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The private refactor preserves validation order and error messages, with no blocking issues identified.
No open issues 🎉
What changed in this PR
Consolidates guest-agent reply validation in the host’s ControlSession, preserving existing behavior and leaving the CLI session unchanged.
Changes:
- Extracts shared acknowledgement, request-ID validation, and error-category helpers.
- Adds tests for invalid acknowledgements and mismatched reply IDs.
| File | Description |
|---|---|
nvx-rs/src/host/session.rs |
Centralizes reply checks and adds regression tests. |
Review effort: Balanced
Pedro Henrique Penna (ppenna)
deleted the
nvx-rs-quality/session-replies
branch
October 20, 2026 13:16
This was referenced Oct 20, 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
ControlSession(nvx::host::session), the host side of a sandbox's control session, checked the guest agent's replies with copies of the same code.pingandstopdiffered only in their request and reply kinds and their message,features,map_host_paths, andnext_exec_eventeach refused a reply to another request with the same check and message, andmap_host_pathsandnext_exec_eventtruncated the error category of a refusal to 64 characters the same way. This change keeps each of those rules once, in three private helpers, so that the requests validate their replies alike by construction and a new request can reuse them. The CLI's own session,nvx::host::cli::session, which keeps the messages ofnvx.py, is untouched.Changes
All in
nvx-rs/src/host/session.rs:ControlSession::acknowledged(kind, reply, what, deadline)sends a request ofkindwith a fresh request ID and no payload, and fails withthe guest agent did not acknowledge WHATunless the next application frame is an emptyreplywith status 0 to that request. The sites that now share it:ping:APP_PING, acknowledged byAPP_READY, asreadiness;stop:APP_STOP, acknowledged byAPP_STOPPED, asstop.ControlSession::read_reply(request_id, deadline)reads the next application frame, fails withthe guest agent returned a mismatched request IDunless it answersrequest_id, and returns its kind, status, and payload. The sites that now share it:features;map_host_paths, for each of its requests;next_exec_event.error_category(payload)returns the error category of anAPP_ERRORpayload, truncated to 64 characters. The sites that now share it:map_host_paths, in its message for a refusal other thanmapping-failed;next_exec_event, inExecEvent::Rejected.pings_and_stops_need_an_empty_acknowledgement: the kinds and empty payloads of the requests thatpingandstopsend, and that each refuses, with its message, a reply of another kind, to another request, with another status, or with a payload;responses_to_another_request_fail_closed: the message with whichfeatures,map_host_paths, andnext_exec_eventrefuse a reply to another request; the existing tests checked only its variant, for the first two.Behavior
Nothing observable changes:
acknowledgedformatsthe guest agent did not acknowledge readinessand... stopfrom what it acknowledges, and the other messages moved verbatim.pingandstopstill test the kind, request ID, status, and payload in one condition with one message, andfeatures,map_host_paths, andnext_exec_eventstill refuse a reply to another request before they look at its kind. Every error is still aSessionError::Protocol, soErrorKinds are unchanged.read_appstill reads and sequences every frame, and each request still draws a fresh request ID.hostfeature, and the CLI's session keeps its own checks.Existing tests drive every changed path: the session's unit tests; the managed agent's loopback test (
guest::managed_agent::loopback), which runsping,features,map_host_paths, workloads and their events, andstopagainst the real agent in-process on Linux;Instance, whose start, exec, and stop the fake-OpenVMM tests ofnvx-rs(tests/fake_openvmm.rs) andaci_edge_sandboxes(tests/openvmm_fake.rs) run; and, on each host below,test-aci-edge-sandboxes, whoseopenvmm_e2etests run the same lifecycle on a real hypervisor, and themanaged-lifecyclescenario oftest-microvm.Follow-ups
The survey turned these up, and this change leaves them alone. I found no suspected bug.
host/cli/session.rsrepeats the same reply checks inping,stop,features, andexec, with themanaged guest ...messages andExiterrors ofnvx.py'sControlSession. Helpers parameterized by message and error could serve both sessions, but they map failures differently, so that wants its own change.host/session.rs, the tests'ScriptedandRespondertransports still have identicalreadmethods.host/platform/linux.rs(start_time_from_stat) andhost/cli/sandbox/identity.rs(linux_start_time) both parse/proc/PID/stat, but they differ observably: the CLI's port of Python requires 20 fields and an all-digit start time and returns Python's messages, while the platform's accepts whatu64::from_straccepts. They want a decision before they share code.host/sandbox/mount.rs,MountAccess,MountOwner, andMountKindrepeatfrom_namewith the same message form, andsandbox/mount/tests.rsandsandbox/tests.rsdefine the samepolicytest helper;process::tests::a_process_runs_until_its_last_thread_exitsandcli::sandbox::identity::tests::a_vm_runs_until_its_last_thread_exitsrepeat the fixture of a Python process whose main thread exits first;filesystem.rs,network/mod.rs, andsandbox/options/tests.rseach define the same option-values test helper;Instance::runtimeandInstance::launchininstance.rsread a record and refuse anotherSTATE_FORMATin two shapes;run_fileandrun_networkinguest/device_io.rsand thedevice_iohelper oftests/guest_tools.rs, the time tools' watcher-violation sites and port-window lock failures, the twoMAPSdecoders ofprotocol/maps.rs, and the guest-program follow-ups that nvx-rs: make the guest failure reporters return their CommandError #614 lists.Validation
830536ed210e0d8bef0ca9166c13fb9636acb614,origin/devwhen the run started: two performance-baseline updates on top of the merge of nvx-rs: parse fake OpenVMM --mount-child values with one function #613.3f97d134bbb0a76f2ea3796c37dcfa47516ae3dd, the tree of this commit. Every host applied the patch to BASE and checked thatgit write-treeequals TREE.v0.1.0-dev.6c4171aa3166, from the merge of nvx-rs: parse fake OpenVMM --mount-child values with one function #613, the nearest first-parent ancestor of BASE that had packages. Its OpenVMM,d0bdbed8f6e3b86271277dc22658109e2e633b2d, is the pinned revision.nvx-rsfrom TREE with Cargo and recorded its provenance, and pre-built theopenvmm_e2eandmanaged_lifecycletest binaries, so that no Cargo build overlapped its hypervisor tests.build-guest-binaries --output;test-guest-binaries --binaries, where 34 tests passed; and the Alpinebuild-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-25654364e7301c11f7048f15c67f10c679f75d5ab5fc3cc1abd9025df6cef4b297e), its packages manifest (9b3bbafc3926715bbd074c398a295691c964db6ca9e4dc7104908cc714b7dd8e), and its provenance (21581b7203f62c333bdd2b580403b179049b81c4db10a6896a2975dfa2bb09b7) went through this machine to the other two hosts, where the SHA-256 digests matched.cargo fmt --check; Clippy with-D warningsfor all targets and features and for the library without default features;cargo testwith all features and with the defaults; and rustdoc with-D warnings. This machine also ran the steps for which the hosts lack a toolchain or target: the Rust 1.89 MSRV check of both crates on Windows, and theaarch64-apple-darwincheck of both crates, which CI runs on Linux.check-guest-binaries; the macOS check ran locallybuild-guest-binaries,test-guest-binaries, andbuild-initramfspassdirectories_on_another_volume_are_mappedpasses by skipping on hosts without a second volume, as on all three.Left to CI: the
test-microvmscenario that runs only when named (smp-lapic), the Ubuntu and Azure Linux guests, the OpenVMM suites, guest determinism, benchmarks, and check-quality and validate-nvx, which this change doesn't affect: it changes no Python, shell, or Markdown file, and adds, moves, or deletes none.