Repository navigation
nvx-rs: send the guest's attachment, ACK, and CREDIT with one method - #684
Merged
Pedro Henrique Penna (ppenna) merged 1 commit intoOct 21, 2026
Merged
Conversation
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>
Contributor
There was a problem hiding this comment.
🟢 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_recordto 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
This was referenced Oct 21, 2026
Pedro Henrique Penna (ppenna)
deleted the
nvx-rs-quality/guest-connection-records
branch
October 21, 2026 21:51
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
GuestConnection, the guest's I/O driver of an NVXS session innvx-rs/src/guest/nvxs.rs, sends three records of one each:attachsends the guest's attachment,acknowledgethe ACK of a reset, andcredita CREDIT. Each spelled out the same five steps around its one call of the guest's session: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 insrcandtests, 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 todevchanges, among themREADME.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, anddoc/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:
sendkeeps 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 intosend_recordwould take a parameter for that difference.send_recordis private and takesimpl FnOnce(&mut GuestSession, &mut Vec<u8>) -> Result<(), SessionError>. The three public methods keep their signatures and documentation, and no public item changes.next_event, thecheckandendhelpers, andHostConnection, 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 withEndedif an earlier failure ended the connection, clears the output buffer, callsencodewith the session and the buffer, maps its error toGuestConnectionError::Protocol, and writes the buffer withwrite.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_recordmakes the calls that each method made, in the same order and with the same arguments:check,output.clear, the session'sattach,acknowledge, orcredit,map_err(GuestConnectionError::Protocol), andwrite. A connection that a failure ended still fails withEndedbefore 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 withIo.tests/nvxs/guest.rs,operations_that_the_state_does_not_allow_change_nothing, in whichcreditandacknowledgeare refused before the attachment andcreditbefore the ACK and beyond the outstanding bytes, without a byte written;the_guest_connection_fails_closed, in whichattachon a broken stream ends the connection andacknowledgeafter a failure fails withEnded; and the echo and record-boundary tests, which attach, acknowledge, and credit;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.rsandhost/sandbox/mount/tests.rsdefine the samepolicyandnativetest helpers; the share tests could import the launch tests' helpers.guest/ingress/address.rsandguest/ingress/collect.rsdefine the samekernel_addresstest helper, which could sit besidedecode_kernel_addressunder#[cfg(test)].ListenerTable::addanddiag::note_wildcard_tcprepeat a bounded find-or-insert merge.protocol/outer.rs'sencode_outerandprotocol/nvxs/record.rs'sencode_recordwrite 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.rsandhost.rs:receiverepeats the same nine lines after its state check in both roles; sharing them needs a trait over each role's parser,accept, andfail.host/sandbox/mount.rs:MountAccess,MountOwner, andMountKindrepeatfrom_namewith 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 Windowsstatandlstatbuild the sameStat, andhost/python/dumps.rs'swrite_valuerepeats its bracket, comma, and indentation loop for lists and dicts.tests/nvxs/broker.rs:accept_guestandaccept_hostrepeat the parse loop, andqueue_guestandqueue_hostthe encoding.host/launch.rs's tests build the same one-childExportPlantwice.host/store/record.rs'shex,aggregate_child_nameinhost/sandbox/mount.rs, andtesting/fake_openvmm.rshex-encode bytes as thehexofhost/cli/sandbox/state.rsdoes, which nvx-rs: separate distro verification from launch planning #653 changes.Validation
BASE:
916a61f0297900db13d7d9d04d170f01ef9ed71f,origin/devwhen the run started. Before this pull request was opened,origin/devadvanced to89620759aby 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 todevchangesnvx-rs/src/guest/nvxs.rs.TREE:
833fd1903b81f5634d367c253cae38a73994048d, 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, the nearest first-parent ancestor of BASE that had packages, two commits earlier. Its OpenVMM,d0bdbed8f6e3b86271277dc22658109e2e633b2d, is the pinned revision, as each host checked inbuild/openvmm.provenance.json.nvx-rs: each host ran the Cargo command with whichnvx.pybuilds it. Cargo found the build innvx-rs/target/releaseup to date with TREE's sources, since the CLI build compiles neither theguestmodule nor the test modules that the previous run changed, and that executable is Cargo's own output, not the release's.record-nvx-rs-provenancethen recorded it, and each host checked that the record names TREE's sources (source_sha25611748de05aa80be86deef267096feeff816adc8d303293d1e7a53a5f3fcecd13) and the executable. Each host built themanaged_lifecycleandopenvmm_e2etest 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 digest605970af7bb70d49dbd4e935e5dec3b020c6e6d444a2d1f1a155ede5e014e0bc;nvx-managed-agenthas SHA-25627b7d2412e46154dc199247ecfed2c9e194891e4d702bc508ce92b46d51a0c23;test-guest-binaries --binaries, where 34 tests passed;build-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-25625d6e65179a5bd2651af5cb4aa0f83eb05e6b4922b81fa24d5fac5e0f3daff5b), 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 warningson all targets with all features and on the library without default features, for Windows and forx86_64-unknown-linux-gnu, which compiles theguestmodule, and on all targets with onlyguestforx86_64-unknown-linux-gnu; andcargo test --all-featureson 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 ofnvx,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 ofnvxandaci_edge_sandboxesand the MSRV check of the runtime adapter's guest-only configuration, which ran as the action defines it, starting with itsrustup 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.test-aci-edge-sandboxestest-microvm, default suitenvxtests 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 85aci_edge_sandboxestests over the two feature sets, and agent bundling (4). The macOS checks and the guest-only MSRV check ran locallynvxtests 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 85aci_edge_sandboxestests over the two feature sets, and agent bundling (4). The MSRV checks ran locallyEvery 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
SIGINTat its default disposition, so the CLI's Ctrl-C tests ran as intended; the sessions' shells ignored onlySIGQUIT(SigIgn: 0x4). 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. The time-ABI evidence lines counted no boot, capture, or restore over its CPU-time budget on any host. On every host, oneopenvmm_e2etest printedskipped: the host has no second volumefor 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 withEPERM; both are properties of the hosts, not of this change.Left to CI:
test-openvmm-unitandtest-openvmm, since OpenVMM is unchanged;test-microvmscenarios that run only when named:smp-lapic, andmanaged-exec-configandsandbox-lifecycle, which need the Ubuntu scratch template that packages omit;verify-guest-determinism;