mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-19 09:15:19 +03:00
2faf7d77db
Stacked on #319, which has PID 1, the channel and the trait but mounts
nothing.
Review that one first; this PR is the descriptor half.
- **The shares are mounted.** A tag names an export, the descriptor
names where
it lands, and every share goes on `nosuid` and `nodev` whether or not it
is
writable — a share is data handed to the guest, and no descriptor has a
way
to ask for a setuid binary or a device node in one. Mounting needs
privileges a test does not have, so the arguments and flags are derived
by a
function the tests assert; that is where the read-only decision lives.
- **Progress is two messages, not one.** `mounted` / `mount_failed` stay
apart
from `started` / `start_failed`, because a share that did not appear and
a
command that did not run want different things looked at. A failure
carries
the reason the operating system gave, verbatim, and the path it happened
on.
- **The second layer is relayed and never read.** Envelopes cross a unix
socket
to the workload and come back the same way. `body` is a string rather
than
nested JSON on purpose: a document this component can index into is a
document it can grow to depend on, and then the layer is not opaque any
more
and the boundary it exists to draw is gone.
- **An envelope is never logged** — not the body, not truncated, not at
debug
level. The channel name and a byte count are the whole of what may be
said
about one. `Payload`'s `Debug` is written by hand for the same reason.
- **A write to a channel nobody reads now ends the session** the same
way a
closed read does, and stops the workload. A caller that stopped
listening has
also stopped being able to say stop; that was two outcomes and is one.
## The tests, failing first
Progress reporting, with the mount result dropped (the state this branch
started from):
```
running 11 tests
test session::tests::an_unreadable_line_does_not_end_a_session ... ok
test session::tests::a_stop_is_idempotent_and_does_not_end_the_session ... ok
test session::tests::the_guest_speaks_first_and_says_its_version ... ok
test session::tests::a_closed_channel_stops_the_workload ... ok
test session::tests::the_descriptor_mounts_and_starts_what_it_names ... ok
test session::tests::an_envelope_crosses_the_session_in_both_directions_unread ... ok
test session::tests::a_share_that_will_not_mount_is_refused_before_anything_starts ... ok
test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... FAILED
test session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount ... FAILED
test session::tests::a_signalled_workload_is_reported_as_signalled ... FAILED
test session::tests::a_relay_nothing_is_on_does_not_end_a_session ... FAILED
failures:
---- session::tests::an_exit_is_reported_and_the_workload_is_not_started_again stdout ----
thread 'session::tests::an_exit_is_reported_and_the_workload_is_not_started_again' (312969) panicked at apps/nesinit/src/session.rs:248:13:
assertion `left == right` failed
left: Started
right: Mounted
---- session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount stdout ----
thread 'session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount' (312963) panicked at apps/nesinit/src/session.rs:460:9:
assertion `left == right` failed
left: StartFailed { reason: "ENOENT: /usr/bin/workload" }
right: Mounted
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- session::tests::a_signalled_workload_is_reported_as_signalled stdout ----
thread 'session::tests::a_signalled_workload_is_reported_as_signalled' (312966) panicked at apps/nesinit/src/session.rs:248:13:
assertion `left == right` failed
left: Started
right: Mounted
---- session::tests::a_relay_nothing_is_on_does_not_end_a_session stdout ----
thread 'session::tests::a_relay_nothing_is_on_does_not_end_a_session' (312964) panicked at apps/nesinit/src/session.rs:248:13:
assertion `left == right` failed
left: Started
right: Mounted
failures:
session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount
session::tests::a_relay_nothing_is_on_does_not_end_a_session
session::tests::a_signalled_workload_is_reported_as_signalled
session::tests::an_exit_is_reported_and_the_workload_is_not_started_again
```
The read-only flag, ignored:
```
running 9 tests
test shutdown::tests::a_workload_that_leaves_in_time_is_not_killed ... ok
test shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power ... ok
test shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes ... ok
test workload::tests::a_failure_names_the_path_it_happened_on ... ok
test workload::tests::a_writable_share_is_still_mounted_without_devices_or_setuid ... ok
test workload::tests::a_read_only_share_is_mounted_read_only ... FAILED
test session::tests::a_closed_channel_stops_the_workload ... ok
test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... ok
test session::tests::a_signalled_workload_is_reported_as_signalled ... ok
failures:
---- workload::tests::a_read_only_share_is_mounted_read_only stdout ----
thread 'workload::tests::a_read_only_share_is_mounted_read_only' (311963) panicked at apps/nesinit/src/workload.rs:212:9:
assertion `left == right` failed
left: 0
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
workload::tests::a_read_only_share_is_mounted_read_only
```
The relay quoting a line it could not decode — which is the second way a
body
reaches a log line, and the reason the failure path logs a length and
nothing
else:
```
running 2 tests
test payload::tests::an_envelope_crosses_in_both_directions_untouched ... ok
test payload::tests::nothing_the_relay_logs_contains_a_body ... FAILED
failures:
---- payload::tests::nothing_the_relay_logs_contains_a_body stdout ----
thread 'payload::tests::nothing_the_relay_logs_contains_a_body' (312716) panicked at apps/nesinit/src/payload.rs:202:9:
a body reached a log line:
2026-09-04T21:11:56.619025Z WARN nesinit::payload: ignoring an envelope that would not decode bytes=62 line={"channel":"identity","body":"a-credential-nobody-should-read"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
payload::tests::nothing_the_relay_logs_contains_a_body
test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 17 filtered out; finished in 0.06s
```
And the first way: a derived `Debug` instead of the hand-written one.
```
running 6 tests
test lifecycle::tests::a_mount_failure_keeps_its_reason_verbatim ... ok
test lifecycle::tests::a_signalled_exit_is_not_a_zero_exit ... ok
test lifecycle::tests::a_line_round_trips ... ok
test lifecycle::tests::defaults_cover_what_a_caller_may_leave_out ... ok
test lifecycle::tests::an_envelope_does_not_print_its_body ... FAILED
test lifecycle::tests::an_envelope_body_stays_a_string_in_both_directions ... ok
failures:
---- lifecycle::tests::an_envelope_does_not_print_its_body stdout ----
thread 'lifecycle::tests::an_envelope_does_not_print_its_body' (313791) panicked at crates/nesprotocol/src/lifecycle.rs:290:9:
the body reached a log line: Payload { channel: "identity", body: "a-credential-nobody-should-read" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
lifecycle::tests::an_envelope_does_not_print_its_body
test result: FAILED. 5 passed; 1 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesprotocol --lib`
```
All green after: 19 unit tests, 3 against real forked children, 15 in
`nesprotocol`.
## What this does not verify
- **Nothing has been mounted.** No `virtiofs` share has been mounted by
this
code, in a VM or anywhere else. What is tested is the source, target and
flag
word handed to the mount call; that the call succeeds against a real
virtio
transport, that the mount point is where a workload then finds its
files, and
that a `uid` mismatch surfaces as the permission error this is written
to
produce, are all unverified.
- **The relay has never carried a real workload's traffic.** Two
processes, a
real unix socket and bytes that come back unchanged is what the test
shows.
Whether the socket path is the right mechanism is openly a guess, and it
is
meant to be replaceable without anything above it moving.
- **"Never logged" is enforced more narrowly than it reads.** The
hand-written
`Debug` and the failure path's length-only line are both tested. The
capture
test cannot reliably see debug-level lines: callsite interest is cached
process-wide, so a line another test in the same binary reached first
never
arrives in the capture. A future `{:?}` on a whole envelope at debug
level
would not necessarily be caught by these tests, only by the `Debug` impl
keeping its shape.
- **`geometry` is parsed and carried, and nothing consumes it.** This
component
does not start the guest's own services yet. `ticket` exists as a
message
with no producer wired to it.
- **Still no VM, still no vsock, still no `uid` drop**, as in #319, and
no
number in this PR is measured.
- **No third-party workload has gone through any of this.** The claim
that a
descriptor plus a set of shares is enough to run something we did not
write
is untested, and our own workload is the weakest possible witness for
it.
## Since review
`d4d473f` — the relay may not stall the session and may not buffer
without end,
plus a descriptor with a nul byte in it is refused by name. Failing
first, in
order:
The session held still behind a workload that was not reading:
```
running 1 test
test session::tests::a_relay_that_is_not_draining_does_not_stall_the_session ... FAILED
failures:
---- session::tests::a_relay_that_is_not_draining_does_not_stall_the_session stdout ----
thread 'session::tests::a_relay_that_is_not_draining_does_not_stall_the_session' (326881) panicked at apps/nesinit/src/session.rs:670:10:
the session stalled on the relay: Elapsed(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
session::tests::a_relay_that_is_not_draining_does_not_stall_the_session
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 5.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
A frame with no end to it:
```
running 1 test
test payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest ... FAILED
failures:
---- payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest stdout ----
thread 'payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest' (327888) panicked at apps/nesinit/src/payload.rs:375:10:
the relay is still assembling a frame that never ends: Elapsed(())
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 5.01s
error: test failed, to rerun pass `-p nesinit --lib`
```
And a tag that was quietly emptied instead of refused:
```
running 1 test
test workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name ... FAILED
failures:
---- workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name stdout ----
thread 'workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name' (327380) panicked at apps/nesinit/src/workload.rs:274:40:
an empty source would have been mounted: ("", "/mnt/user", 6)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
One behaviour changed rather than only hardened, and it is worth a
reviewer's
eye: **nothing is queued for a workload that is not on the relay.** An
envelope
that arrives with nobody connected is dropped, as is one that arrives
faster
than the workload reads. That follows the layer's own rule — what
crosses it is
re-sent when it changes, so a held copy is a stale copy — but it does
mean a
sender that assumes delivery is wrong to. Nothing here retries, and
nothing
tells the far end that a particular envelope was dropped.
23 unit tests, 5 against real forked children, 15 in `nesprotocol`.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR mounts descriptor-defined virtiofs shares, reports mount and
process-start progress independently, and relays opaque payload
envelopes between the host channel and workload Unix socket. Changes
since the previous review also bound relay frames, prevent relay
backpressure from stalling lifecycle handling, reject descriptor strings
containing NUL bytes, and track whether a reaped PID remains valid.
- Mounts shares at descriptor-selected targets with `nosuid`, `nodev`,
and optional read-only flags.
- Adds bidirectional opaque payload forwarding with bounded,
non-blocking queues and body-safe logging.
- Adds distinct mounted/start lifecycle responses and failure reporting.
- Adds capped newline-delimited relay frames and drops stale or
backpressured envelopes.
- Reworks workload tracking to avoid signaling a PID after its exit has
been delivered.
<h3>Confidence Score: 5/5</h3>
The reviewed changes appear safe to merge, with no accepted new findings
or outstanding previous root-thread findings.
The resolved relay-stall, unbounded-frame, and invalid-NUL findings are
addressed by non-blocking delivery, capped frame assembly, and explicit
descriptor validation. The protocol-version concern was correctly
withdrawn under the coordinated version-2 deployment model. The
remaining PID check-to-signal race duplicates an existing prior comment
and therefore is not reposted or counted as a new finding.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/nesinit/src/payload.rs | Adds the bounded, bidirectional
Unix-socket payload relay with non-blocking delivery and body-safe
logging. |
| apps/nesinit/src/session.rs | Integrates payload events with lifecycle
handling and separately reports mount and process-start outcomes. |
| apps/nesinit/src/workload.rs | Implements descriptor-driven virtiofs
mounts and switches process signaling to tracked reaper state. |
| apps/nesinit/src/reap.rs | Adds shared reaped-state tracking so
callers stop treating a delivered PID as the workload. |
| crates/nesprotocol/src/lifecycle.rs | Extends lifecycle messages with
mount progress and opaque payload envelopes while redacting payload
bodies from Debug output. |
| apps/nesinit/src/main.rs | Starts the payload relay before the
workload session and wires bounded relay ports into session handling. |
| apps/nesinit/README.md | Documents mount behavior, payload opacity,
delivery semantics, frame limits, and progress reporting. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant H as Host
participant N as nesinit
participant M as virtiofs mounts
participant W as Workload
H->>N: Boot descriptor
N->>M: Mount descriptor shares
M-->>N: Success or failure
N-->>H: mounted / mount_failed
N->>W: Start command
N-->>H: started / start_failed
H->>N: Payload envelope
N-->>W: Non-blocking Unix-socket relay
W->>N: Payload envelope
N-->>H: Payload envelope
W-->>N: Exit
N-->>H: workload_exited
```
<sub>Reviews (3): Last reviewed commit: ["fix(nesinit): the relay may
not stall
th..."](e94ea00593)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=60585116)</sub>
**Context used:**
- Knowledge Base — [Streaming appliance
build](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/appliance-build.md)
<!-- /greptile_comment -->