Skip to content

nvx-rs: check the CLI session's replies through shared methods - #680

Merged
Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/cli-session-replies
Oct 21, 2026
Merged

Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/cli-session-replies

Conversation

@ppenna

Copy link
Copy Markdown
Contributor

Summary

Session, the control session of the nvx-rs CLI (nvx-rs/src/host/cli/session.rs), checked the guest agent's replies with copies of the same code. ping and stop were the same eight lines apart from their request and reply kinds and the word in their message, and features and exec each 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 ControlSession in nvx::host::session, which Session wraps: acknowledged, the exchange of ping and stop, and reply, 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.rs adds:
    • Session::reply(request_id, deadline), which reads the next response with receive and returns its kind, status, and payload, or fails with managed guest returned a mismatched request ID if it answers another request;
    • Session::acknowledged(kind, reply, what, timeout), which draws a request ID, sets the deadline, sends an empty request of kind, reads one response, and fails with managed guest did not acknowledge {what} unless it is an empty reply with status 0 to that request.
  • Every site that now shares them:
    • ping calls acknowledged(APP_PING, APP_READY, "readiness", timeout);
    • stop calls acknowledged(APP_STOP, APP_STOPPED, "stop", timeout);
    • features and the response loop of exec read with reply(request_id, deadline) in place of receive and their copies of the request-ID check.
  • nvx-rs/src/host/cli/session/tests.rs: protocol_violations_keep_the_messages_of_python also checks the mismatched-request-ID message of features and 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:

  • Each operation makes the same calls in the same order: request_id(), Deadline::after(timeout), send, then receive, 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, and exec still checks the request ID of every response before it looks at its kind.
  • The messages are the same strings: acknowledged formats managed guest did not acknowledge readiness and managed guest did not acknowledge stop from its what, and reply returns the message that both of its copies returned.
  • Neither method closes the session when it refuses a reply, as the copies did not; only a failed transfer does, in send or receive.
  • Session is pub(crate) and its operations keep their signatures, so neither the CLI's commands nor aci_edge_sandboxes see anything new. Only the cli feature compiles this module.

The session's unit tests already covered the acknowledgements of ping and stop, with a reply to another request and a reply of the wrong kind, and the request-ID check of exec. The golden CLI scenarios and the other tests of tests/cli.rs start, run, and stop sandboxes of nvx-fake-openvmm through the nvx-rs binary, which drives ping, features, exec, and stop. The new assertions pin the cases that no test reached, and they also pass at BASE.

Follow-ups

  • features, exec, and acknowledged each still open with the same three steps: draw a request ID, set the deadline, and send the request. A request(kind, payload, timeout) method could take them and return the ID and the deadline.
  • GuestConnection in nvx-rs/src/guest/nvxs.rs: attach, acknowledge, and credit repeat 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_outer in nvx-rs/src/protocol/outer.rs writes the 44-byte NVXS header field by field as nvxs::encode_record does, and encode_ack_payload and decode_ack_payload, like the two credit functions, repeat their range checks. One header writer could serve both codecs.
  • MountAccess, MountOwner, and MountKind in nvx-rs/src/host/sandbox/mount.rs repeat from_name with one message shape.
  • About ten places hex-encode bytes with format!("{byte:02x}"), among them host/store/record.rs, host/cli/sandbox/state.rs, host/sandbox/mount.rs, host/sandbox/options.rs, host/platform/windows.rs, and testing/fake_openvmm.rs. Open pull requests change several of those files.

Validation

  • BASE: 916a61f0297900db13d7d9d04d170f01ef9ed71f, origin/dev when 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 to dev changes the files of this one.

  • TREE: bfd30bb9f4eea711c797d9c816b3d5c7e32c7687, 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.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.

  • download installed the release's nvx-rs, whose provenance names BASE's nvx-rs sources, which TREE changes. Each host then deleted it, built nvx-rs from TREE with the Cargo command that nvx.py uses, recorded its provenance with record-nvx-rs-provenance, and checked that the record names TREE's sources (source_sha256 7058d66f70389a2d4f4aae71b0bbabb43a28be79c9ac23683b91e4fe9161bf78) and the executable that it built. After its other Cargo work, each host also built the openvmm_e2e and managed_lifecycle test 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;
    • the Alpine build-initramfs with NVX_GUEST_BINARIES.

    initramfs.cpio.gz (SHA-256 2d12a797f23e00d7363ba9290f7efdb86b0b401227a380f6bc13c8e58f690f4b), 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:

    • rustfmt; Clippy with -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;
    • the new assertions against BASE's session.rs, which they also pass;
    • the steps that no host could run, because the hosts lack the toolchain or target: the Windows MSRV (1.89) checks of nvx, aci_edge_sandboxes, and the runtime adapter's protocol-only and host-only configurations; the macOS (aarch64-apple-darwin) checks of nvx and aci_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 its rustup 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 with RUSTUP_AUTO_INSTALL=0.

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, including the MSRV (1.89) checks, 736 nvx tests with all features, among them tests/cli.rs and 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), 243 aci_edge_sandboxes tests over both feature sets, and agent bundling (4). The macOS checks and the guest-only MSRV check 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 (34 tests) Pass (18/18) Pass (41 scenario runs)
prometheus28 whp baremetal Pass: every Windows step, including 429 nvx tests with all features, among them tests/cli.rs and 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), 243 aci_edge_sandboxes tests over both feature sets, and agent bundling (4). The MSRV checks ran locally Copied in, digests match Pass (18/18) Pass (41 scenario runs)

Every 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's managed-lifecycle scenario ran the managed_lifecycle test with Cargo, which passed. OpenVMM warned of a host CPU-profile fallback only in the cpu-profile-fallback scenario, which forces one, and the second-volume case of test-aci-edge-sandboxes skipped 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-exit scenario, used 12,725 µs of CPU against its 12,000 µs budget, and on prometheus30 one of 58, in the 8-vCPU time-abi-conformance scenario, 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:

  • the OpenVMM unit and VMM suites, test-openvmm-unit and test-openvmm;
  • the microVM scenarios that run only when named: smp-lapic, and managed-exec-config and sandbox-lifecycle, which drive nvx.py sandbox through nvx-rs and so run this session against real guests, and which need the Ubuntu scratch template that packages omit;
  • the debug-kernel scenarios, 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.

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>
Copilot AI balanced review requested due to automatic review settings October 21, 2026 06:49

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 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

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