Skip to content

nvx-rs: send the guest's attachment, ACK, and CREDIT with one method - #684

Merged
Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/guest-connection-records
Oct 21, 2026
Merged

Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/guest-connection-records

Conversation

@ppenna

Copy link
Copy Markdown
Contributor

Summary

GuestConnection, the guest's I/O driver of an NVXS session in nvx-rs/src/guest/nvxs.rs, sends three records of one each: attach sends the guest's attachment, acknowledge the ACK of a reset, and credit a CREDIT. Each spelled out the same five steps around its one call of the guest's session:

self.check()?;
self.output.clear();
self.session
    .credit(bytes, &mut self.output)
    .map_err(GuestConnectionError::Protocol)?;
self.write()

A private method, send_record(encode), now holds those steps and takes the session call that encodes the record, so each of the three methods names only the record that it sends. How the connection sends one record has one definition: refuse once a failure ended the connection, encode the record into the cleared buffer, report a record that the session refuses as a protocol error without ending the connection, and write it, ending the connection if the write fails. #678, #680, #682, and #683 listed this as a follow-up.

This is the twenty-sixth nvx-rs-quality/ pull request. The 19 from #603 to #637 merged, and #678 through #683 are open. Each had a Copilot overview review that recommended approval with no comment to follow, and none was closed without merging. This run surveyed the parts of the crate that they left alone, by reading, with Clippy's pedantic lints as hints, and with a throwaway script, kept outside the repository, that reports every window of six or more whitespace-normalized lines that repeats in src and tests, ignoring blank lines, comments, and lines of punctuation, exactly and with names and literals abstracted, and once more over production code alone. It skipped every file that an open pull request to dev changes, among them README.md, Cargo.toml, src/lib.rs, host/mod.rs, the managed agent, guest/mod.rs, guest/sys, guest/time, guest/time_probe, the guest programs' entry points, host/instance.rs, host/process.rs, host/filesystem.rs, host/platform, host/store/mod.rs, host/sandbox/{mod,options,distro}.rs, host/cli/session.rs, host/cli/sandbox/{lifecycle,mod,state}.rs, the golden tests, the NVXS unit tests, tests/cli.rs, and doc/project-structure.md.

Of the candidates in the remaining files, this one gives the most production behavior one definition per changed line: 28 changed lines for the sending of three records. The duplicated test helpers that the survey found remove more lines per changed line, about 10 in 18, but they repeat only fixtures. The other production candidates save no lines or need a new trait. Follow-ups lists them.

Where the choice was open, I took the conservative option:

  • send keeps its own loop. Its DATA records may take several writes, and a refusal after one of them ends the connection, which the three methods never do; folding it into send_record would take a parameter for that difference.
  • send_record is private and takes impl FnOnce(&mut GuestSession, &mut Vec<u8>) -> Result<(), SessionError>. The three public methods keep their signatures and documentation, and no public item changes.
  • next_event, the check and end helpers, and HostConnection, whose attachment waits for the broker's answer, stay as they are.

Changes

All changes are in nvx-rs/src/guest/nvxs.rs: 13 insertions and 15 deletions.

  • GuestConnection::send_record(encode), private, with a doc comment: fails with Ended if an earlier failure ended the connection, clears the output buffer, calls encode with the session and the buffer, maps its error to GuestConnectionError::Protocol, and writes the buffer with write.
  • The sites that now share it:
    • attach: self.send_record(GuestSession::attach);
    • acknowledge: self.send_record(|session, output| session.acknowledge(window, output));
    • credit: self.send_record(|session, output| session.credit(bytes, output)).

No public item, message, error kind, protocol byte, fixture, or other file changes.

Behavior

Unchanged:

  • send_record makes the calls that each method made, in the same order and with the same arguments: check, output.clear, the session's attach, acknowledge, or credit, map_err(GuestConnectionError::Protocol), and write. A connection that a failure ended still fails with Ended before anything is encoded, a record that the session refuses still writes nothing and leaves the connection usable, and a write that fails still ends the connection with Io.
  • Tests that cover it, none of them added, removed, skipped, or weakened:
    • in tests/nvxs/guest.rs, operations_that_the_state_does_not_allow_change_nothing, in which credit and acknowledge are refused before the attachment and credit before the ACK and beyond the outstanding bytes, without a byte written; the_guest_connection_fails_closed, in which attach on a broken stream ends the connection and acknowledge after a failure fails with Ended; and the echo and record-boundary tests, which attach, acknowledge, and credit;
    • the runtime adapter's 21 tests in its musl guest-only configuration, whose guest agent calls all three;
    • every microVM scenario: the managed agent, the guest program that uses GuestConnection, was rebuilt from this tree into the Alpine initramfs and calls all three in each session with OpenVMM's broker.

Follow-ups

Not in this pull request, to keep it to one change. I found no suspected bug.

  • host/sandbox/tests.rs and host/sandbox/mount/tests.rs define the same policy and native test helpers; the share tests could import the launch tests' helpers.
  • guest/ingress/address.rs and guest/ingress/collect.rs define the same kernel_address test helper, which could sit beside decode_kernel_address under #[cfg(test)]. ListenerTable::add and diag::note_wildcard_tcp repeat a bounded find-or-insert merge.
  • protocol/outer.rs's encode_outer and protocol/nvxs/record.rs's encode_record write the 44-byte NVXS header field by field. One writer would give the header layout one definition, though it saves no lines.
  • protocol/nvxs/guest.rs and host.rs: receive repeats the same nine lines after its state check in both roles; sharing them needs a trait over each role's parser, accept, and fail.
  • host/sandbox/mount.rs: MountAccess, MountOwner, and MountKind repeat from_name with one message shape; a helper that takes the setting's name and its two values would save about 4 lines in 40.
  • host/python/fs.rs's Windows stat and lstat build the same Stat, and host/python/dumps.rs's write_value repeats its bracket, comma, and indentation loop for lists and dicts.
  • tests/nvxs/broker.rs: accept_guest and accept_host repeat the parse loop, and queue_guest and queue_host the encoding. host/launch.rs's tests build the same one-child ExportPlan twice.
  • host/store/record.rs's hex, aggregate_child_name in host/sandbox/mount.rs, and testing/fake_openvmm.rs hex-encode bytes as the hex of host/cli/sandbox/state.rs does, which nvx-rs: separate distro verification from launch planning #653 changes.
  • The follow-ups that nvx-rs: check the golden tests' fixture sections with one function #683 lists as waiting on files that open pull requests change remain.

Validation

  • BASE: 916a61f0297900db13d7d9d04d170f01ef9ed71f, origin/dev when the run started. Before this pull request was opened, origin/dev advanced to 89620759a by the merge of performance: keep results measured before a history reset out of the series #658, which changes the performance scripts, their tests, and their documentation, none of this pull request's files, and no open pull request to dev changes nvx-rs/src/guest/nvxs.rs.

  • TREE: 833fd1903b81f5634d367c253cae38a73994048d, 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, the nearest first-parent ancestor of BASE that had packages, two commits earlier. Its OpenVMM, d0bdbed8f6e3b86271277dc22658109e2e633b2d, is the pinned revision, as each host checked in build/openvmm.provenance.json.

  • nvx-rs: each host ran the Cargo command with which nvx.py builds it. Cargo found the build in nvx-rs/target/release up to date with TREE's sources, since the CLI build compiles neither the guest module nor the test modules that the previous run changed, and that executable is Cargo's own output, not the release's. record-nvx-rs-provenance then recorded it, and each host checked that the record names TREE's sources (source_sha256 11748de05aa80be86deef267096feeff816adc8d303293d1e7a53a5f3fcecd13) and the executable. Each host built the managed_lifecycle and openvmm_e2e test binaries after its other Cargo work and before its hypervisor tests, whose logs show no compilation.

  • Guest artifacts were rebuilt from TREE on prometheus30, whose user namespaces let Alpine's apk run its scripts (prometheus32 denies them):

    • build-guest-binaries --output, from guest sources with digest 605970af7bb70d49dbd4e935e5dec3b020c6e6d444a2d1f1a155ede5e014e0bc; nvx-managed-agent has SHA-256 27b7d2412e46154dc199247ecfed2c9e194891e4d702bc508ce92b46d51a0c23;
    • test-guest-binaries --binaries, where 34 tests passed;
    • the Alpine build-initramfs with NVX_GUEST_BINARIES.

    initramfs.cpio.gz (SHA-256 25d6e65179a5bd2651af5cb4aa0f83eb05e6b4922b81fa24d5fac5e0f3daff5b), its packages manifest (586bcafdf4d21339c832d1948cd4d977993f050bef4b4b1cce4afaefad2fe42c), and its provenance (416229507943eb6de3f6749bb64b391fe7e31c6ca197c47dad19c66e78964087) went through this machine to the other two hosts, where the SHA-256 digests matched. The image differs from the one that nvx-rs: check the golden tests' fixture sections with one function #683's run built from the same BASE, because its guest binaries hold this change.

  • Local checks with Rust 1.95.0 on Windows: rustfmt; Clippy with -D warnings on all targets with all features and on the library without default features, for Windows and for x86_64-unknown-linux-gnu, which compiles the guest module, and on all targets with only guest for x86_64-unknown-linux-gnu; and cargo test --all-features on WSL Ubuntu. This machine also ran 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, on Windows; and, on WSL Ubuntu, 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 ran as the action defines it, starting with its rustup target add, which gave the local 1.89 the musl target. I removed that 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.

  • A harness note: the first run of the Windows gates ran no command, because Windows PowerShell's built-in alias r (Invoke-History) shadowed the script's helper function of that name, and every step reported success in 0 seconds, which gave it away. I renamed the helper, made a step fail unless it ran a command, and reran every Windows step; the table reports that run.

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, 752 nvx tests with all features (13 of them the NVXS integration tests) and 362 with the default ones, artifact bundling (15), check-guest-binaries (397 tests), the runtime adapter's protocol-only, host-only, and musl guest-only configurations (13, 14, and 21 tests), 158 and 85 aci_edge_sandboxes tests over the two 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 and 358 with the default ones, artifact bundling (15), the runtime adapter's protocol-only and host-only configurations (13 and 14 tests), 158 and 85 aci_edge_sandboxes tests over the two 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 hypervisor test passed at its first run, counting the Windows gates from the rerun that the harness note describes. The Linux gates ran in the foreground of their SSH sessions with SIGINT at its default disposition, so the CLI's Ctrl-C tests ran as intended; the sessions' shells ignored only SIGQUIT (SigIgn: 0x4). 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. The time-ABI evidence lines counted no boot, capture, or restore over its CPU-time budget on any host. On every host, one openvmm_e2e test printed skipped: the host has no second volume for a check that needs one, and on the Linux hosts the filesystem caller-ownership scenario reported that OpenVMM cannot assume other host identities, so it checked that guest root and foreign guest callers fail with EPERM; both are properties of the hosts, not of this change.

Left to CI:

  • the OpenVMM unit and VMM suites, test-openvmm-unit and test-openvmm, since OpenVMM is unchanged;
  • the test-microvm scenarios that run only when named: smp-lapic, and managed-exec-config and sandbox-lifecycle, which need the Ubuntu scratch template that packages omit;
  • the debug-kernel scenarios, the Ubuntu and Azure Linux guests, the Docker guest build, and verify-guest-determinism;
  • the benchmarks and performance gates;
  • the virtual-machine runners;
  • check-quality and validate-nvx, which no Python, shell, or Markdown change touches.

GuestConnection::attach, acknowledge, and credit each repeated the same
five steps around one call of the guest's session: fail if an earlier
failure ended the connection, clear the output buffer, encode the
record through the session, report a record that the session refuses
as a protocol error, and write the record, ending the connection if
the write fails.

send_record now holds those steps and takes the session call that
encodes the record, so each of the three methods only names the record
that it sends: GuestSession::attach, or a closure that passes the
receive window or the credited bytes. Sending one record has one
definition. send keeps its own loop, because its DATA records may take
several writes and a refusal after one of them ends the connection.

Behavior is unchanged: send_record makes the same calls in the same
order with the same error mapping, so a refused record still writes
nothing and leaves the connection usable, and a failed write still ends
it. The NVXS integration tests in tests/nvxs/guest.rs drive every path
of the three methods, the runtime adapter's guest-session tests drive
its guest agent through them, and the managed agent calls all three in
every session with OpenVMM's broker.

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

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 extraction preserves operation order and error handling, with no unresolved findings.

No open issues 🎉

What changed in this PR

Consolidates single-record sending in the NVXS guest connection without changing public APIs or failure behavior.

Changes:

  • Adds private send_record to share connection checks, encoding, and writing.
  • Routes attachment, ACK, and CREDIT through it, leaving DATA sending unchanged.
File Description
nvx-rs/​src/​guest/​nvxs.rs Replaces three repeated send sequences with one private helper.

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