Repository navigation
nvx-rs: check the CLI session's replies through shared methods - #680
Merged
Pedro Henrique Penna (ppenna) merged 1 commit intoOct 21, 2026
Merged
Conversation
Session, the control session of the nvx-rs CLI, 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 drew a request ID, sent an empty request, read the next response, and failed unless it was an empty reply of the expected kind, with status 0, to that request. features and exec each read a response and refused one that answered another request with the same message. Two private methods now hold these rules once, in the shapes that #615 gave ControlSession, which Session wraps: acknowledged, the exchange of ping and stop, and reply, the read that refuses a response to another request. Each check lives in one place, so the operations cannot drift apart in how they validate their replies, and the CLI's session reads like the library's. Behavior is unchanged. Every message is the same, including "managed guest did not acknowledge readiness" and "... stop", which acknowledged now formats from what it acknowledges; each operation makes the same calls in the same order with the same deadline; and a refused reply still leaves the session open. The session's unit tests and the golden CLI scenarios of tests/cli.rs, which start, run, and stop sandboxes of nvx-fake-openvmm through the nvx-rs binary, drive every changed path. The test of the Python messages now also pins the request-ID check of features and the refusal of an acknowledgement with a nonzero status or a payload, cases that no test reached and that pass without this change too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused refactor preserves validation order, deadlines, and error handling, with no unresolved findings.
No open issues 🎉
What changed in this PR
Consolidates the CLI session’s reply validation into private helpers, following the library session’s pattern while preserving behavior.
Changes:
- Shares acknowledgement and request-ID checks across session operations.
- Adds regression assertions for mismatched IDs and invalid acknowledgements.
| File | Description |
|---|---|
nvx-rs/src/host/cli/session/tests.rs |
Adds protocol-error regression assertions. |
nvx-rs/src/host/cli/session.rs |
Extracts shared reply-validation helpers. |
Review effort: Balanced
Pedro Henrique Penna (ppenna)
deleted the
nvx-rs-quality/cli-session-replies
branch
October 21, 2026 15:39
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
Session, the control session of thenvx-rsCLI (nvx-rs/src/host/cli/session.rs), checked the guest agent's replies with copies of the same code.pingandstopwere the same eight lines apart from their request and reply kinds and the word in their message, andfeaturesandexeceach followed their read of a response with the same five-line check of its request ID.This adds two private methods that hold those rules once, in the shapes that #615 gave
ControlSessioninnvx::host::session, whichSessionwraps:acknowledged, the exchange ofpingandstop, andreply, the read that refuses a response to another request. Each check now lives in one place, so the operations cannot drift apart in how they validate their replies, and the CLI's session reads like the library's.Changes
nvx-rs/src/host/cli/session.rsadds:Session::reply(request_id, deadline), which reads the next response withreceiveand returns its kind, status, and payload, or fails withmanaged guest returned a mismatched request IDif it answers another request;Session::acknowledged(kind, reply, what, timeout), which draws a request ID, sets the deadline, sends an empty request ofkind, reads one response, and fails withmanaged guest did not acknowledge {what}unless it is an emptyreplywith status 0 to that request.pingcallsacknowledged(APP_PING, APP_READY, "readiness", timeout);stopcallsacknowledged(APP_STOP, APP_STOPPED, "stop", timeout);featuresand the response loop ofexecread withreply(request_id, deadline)in place ofreceiveand their copies of the request-ID check.nvx-rs/src/host/cli/session/tests.rs:protocol_violations_keep_the_messages_of_pythonalso checks the mismatched-request-ID message offeaturesand the refusal of an acknowledgement with a nonzero status or with a payload.git diff --shortstat: 2 files changed, 52 insertions(+), 28 deletions(-).Behavior
Unchanged:
request_id(),Deadline::after(timeout),send, thenreceive, which records the transfer's direction for the timeout messages and closes the session if the transfer fails, then the same checks. The deadlines are the same, andexecstill checks the request ID of every response before it looks at its kind.acknowledgedformatsmanaged guest did not acknowledge readinessandmanaged guest did not acknowledge stopfrom itswhat, andreplyreturns the message that both of its copies returned.sendorreceive.Sessionispub(crate)and its operations keep their signatures, so neither the CLI's commands noraci_edge_sandboxessee anything new. Only theclifeature compiles this module.The session's unit tests already covered the acknowledgements of
pingandstop, with a reply to another request and a reply of the wrong kind, and the request-ID check ofexec. The golden CLI scenarios and the other tests oftests/cli.rsstart, run, and stop sandboxes ofnvx-fake-openvmmthrough thenvx-rsbinary, which drivesping,features,exec, andstop. The new assertions pin the cases that no test reached, and they also pass at BASE.Follow-ups
features,exec, andacknowledgedeach still open with the same three steps: draw a request ID, set the deadline, and send the request. Arequest(kind, payload, timeout)method could take them and return the ID and the deadline.GuestConnectioninnvx-rs/src/guest/nvxs.rs:attach,acknowledge, andcreditrepeat the same five lines (check the connection, clear the output, encode with the session, and write it). One private method that takes the encoding step could replace them.encode_outerinnvx-rs/src/protocol/outer.rswrites the 44-byte NVXS header field by field asnvxs::encode_recorddoes, andencode_ack_payloadanddecode_ack_payload, like the two credit functions, repeat their range checks. One header writer could serve both codecs.MountAccess,MountOwner, andMountKindinnvx-rs/src/host/sandbox/mount.rsrepeatfrom_namewith one message shape.format!("{byte:02x}"), among themhost/store/record.rs,host/cli/sandbox/state.rs,host/sandbox/mount.rs,host/sandbox/options.rs,host/platform/windows.rs, andtesting/fake_openvmm.rs. Open pull requests change several of those files.Validation
BASE:
916a61f0297900db13d7d9d04d170f01ef9ed71f,origin/devwhen the run started and still when this pull request was opened: a performance-baseline update after the merge of nvx-rs: run the control session on the shared NVXS session's host role #641. No open pull request todevchanges the files of this one.TREE:
bfd30bb9f4eea711c797d9c816b3d5c7e32c7687, 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.dad3e41d7f05, from the merge of nvx-rs: run the control session on the shared NVXS session's host role #641, the nearest first-parent ancestor of BASE that had packages, two commits earlier. Its OpenVMM,d0bdbed8f6e3b86271277dc22658109e2e633b2d, is the pinned revision.downloadinstalled the release'snvx-rs, whose provenance names BASE'snvx-rssources, which TREE changes. Each host then deleted it, builtnvx-rsfrom TREE with the Cargo command thatnvx.pyuses, recorded its provenance withrecord-nvx-rs-provenance, and checked that the record names TREE's sources (source_sha2567058d66f70389a2d4f4aae71b0bbabb43a28be79c9ac23683b91e4fe9161bf78) and the executable that it built. After its other Cargo work, each host also built theopenvmm_e2eandmanaged_lifecycletest binaries, and its hypervisor test logs show no compilation.Guest artifacts were rebuilt from TREE on prometheus30, whose user namespaces let Alpine's apk run its scripts:
build-guest-binaries --output;test-guest-binaries --binaries, where 34 tests passed;build-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-2562d12a797f23e00d7363ba9290f7efdb86b0b401227a380f6bc13c8e58f690f4b), its packages manifest, and its provenance went through this machine to the other two hosts, where the SHA-256 digests matched.Local checks on Windows with Rust 1.95.0:
-D warnings, with all targets and features and on the library without default features; the tests with all features and with the default ones; and rustdoc with-D warnings;session.rs, which they also pass;nvx,aci_edge_sandboxes, and the runtime adapter's protocol-only and host-only configurations; the macOS (aarch64-apple-darwin) checks ofnvxandaci_edge_sandboxes; and the MSRV check of the runtime adapter's guest-only configuration, which this machine ran as the action defines it, starting with itsrustup target add, which gave the local 1.89 the musl target. I removed the target again afterwards.The change adds, moves, and deletes no file and changes no Markdown, so the CLI tests of validate-nvx and the link check had nothing to cover.
One deviation, now undone: my first probe of prometheus28 listed the targets of
1.89, which that host does not have, and rustup installed it to answer. I uninstalled it at once, before any gate ran, so the host keeps only its own toolchains, and its MSRV steps ran on this machine. Every later remote step ran withRUSTUP_AUTO_INSTALL=0.test-aci-edge-sandboxestest-microvm, default suitenvxtests with all features, among themtests/cli.rsand the CLI session's, and 359 with the default ones, artifact bundling (15),check-guest-binaries(384 tests), the runtime adapter's protocol-only, host-only, and musl guest-only configurations (13, 14, and 21 tests), 243aci_edge_sandboxestests over both feature sets, and agent bundling (4). The macOS checks and the guest-only MSRV check ran locallynvxtests with all features, among themtests/cli.rsand the CLI session's, and 358 with the default ones, artifact bundling (15), the runtime adapter's protocol-only and host-only configurations (13 and 14 tests), 243aci_edge_sandboxestests over both feature sets, and agent bundling (4). The MSRV checks ran locallyEvery gate and test passed at its first attempt. The Linux gates ran in the foreground of their SSH session with no signal ignored (
SigIgn: 0), so the CLI's Ctrl-C tests ran as intended. Each suite'smanaged-lifecyclescenario ran themanaged_lifecycletest with Cargo, which passed. OpenVMM warned of a host CPU-profile fallback only in thecpu-profile-fallbackscenario, which forces one, and the second-volume case oftest-aci-edge-sandboxesskipped itself on every host, none of which has a second volume.The time-ABI evidence lines counted no capture or restore check over its CPU-time budget on any host, and no boot check over budget on prometheus28. On prometheus32, one of 58 boot checks, in the capture of the 4-vCPU
console-exitscenario, used 12,725 µs of CPU against its 12,000 µs budget, and on prometheus30 one of 58, in the 8-vCPUtime-abi-conformancescenario, used 10,875 µs against 10,000 µs; CI reports these counts and gates on none of them. TREE changes no file that the guest binaries compile, so their checks run the code of BASE.Left to CI:
test-openvmm-unitandtest-openvmm;smp-lapic, andmanaged-exec-configandsandbox-lifecycle, which drivenvx.py sandboxthroughnvx-rsand so run this session against real guests, and which need the Ubuntu scratch template that packages omit;