Skip to content

Container Dev Mode: host CLI, embedded registry, and VM push path - #184

Open
jetm wants to merge 41 commits into
mainfrom
container-dev-mode
Open

Container Dev Mode: host CLI, embedded registry, and VM push path#184
jetm wants to merge 41 commits into
mainfrom
container-dev-mode

Conversation

@jetm

@jetm jetm commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Container Dev Mode is the inner dev loop for a containerized app on an
immutable Avocado OS device: edit on the host, and only the changed image
layer is hot-reloaded onto the running device — no reflash, no full re-ship.

This adds the host half:

  • a per-project content-addressed OCI store, split write/read listeners, and
    a control WebSocket (three-listener model), all over a per-project pinned CA;
  • Basic-gated write token vs Bearer read/control token (split-credential model);
  • an engine-driver watcher (docker/podman) that syncs only changed layers and
    selects PUSH vs INGEST by host topology, plus a cross-arch guard;
  • avocado container dev up/sync/status/down/prune;
  • the authenticated VM push path: on the macOS/VM fast path the guest pushes to
    a routable HTTPS write listener whose per-project CA is delivered at up,
    never baked.

Verification: unit + integration suites (container_dev lib, plus
container_dev_e2e, container_dev_security, container_dev_arch) all pass;
the VM write path was validated end-to-end 8/8 against a local QEMU docker+ssh
engine VM. Phase 0 de-risk findings are recorded in
docs/container-dev/phase0-findings.md.

jetm added 24 commits July 21, 2026 15:11
Container Dev Mode is a safety-critical change whose Phase 0 gate must
de-risk the core premises before any Phase 1 code ships. The evidence
was scattered across an in-session spike and a maintainer's lab, with no
durable record of what was actually verified versus attested.

Record the Phase 0 decision as GO with explicit provenance: the
two-socket separation plus auth matrix (1.11) and the docker arm of the
loopback credential path (1.4) were verified in-session with tool
output; the remaining hardware spikes (1.1-1.10) are maintainer-attested
in-lab. Separating verified from attested keeps the gate honest at this
risk tier.

Signed-off-by: Javier Tia <javier@peridio.com>
There is currently no foundation for the planned Container Dev Mode
feature, which requires an embedded OCI Distribution registry server
with TLS and WebSocket support running inside the CLI. Without the
necessary dependencies and module structure in place, subsequent tasks
implementing config parsing, registry listeners, engine-driver watching,
and sync orchestration have nowhere to build on.

Introduce the axum, rustls, tokio-rustls, rcgen, and
tokio-tungstenite crates as dependencies, all pinned to the
aws-lc-rs crypto provider already present via reqwest. This avoids
pulling in a second C-based crypto library and keeps the build
requirement footprint unchanged. A new container_dev module is added
under src/utils as scaffolding, exposing a clear location for the
registry server surface and dev loop logic to land in follow-up tasks.

Signed-off-by: Javier Tia <javier@peridio.com>
Without a typed configuration structure, the container_dev feature has
no way to be selectively enabled per runtime or carry validated
parameters such as the watched image list and registry port. Any
downstream implementation task would be building on an undefined
interface.

Introduce ContainerDevConfig as the canonical typed representation of
the runtimes.<name>.container_dev YAML block, and wire it into
RuntimeConfig as an optional field. Presence of the field enables the
feature for that runtime; absence leaves it off. This structural gate
is intentional — a container_dev block placed anywhere other than under
a runtime is ignored by the parser. The default registry port is set to
5599, explicitly avoiding 5000 which conflicts with the macOS AirPlay
Receiver as established during phase 0 task 1.6. The key is also
registered with the external-config-ref scanner so the scanner does not
recurse into the images list and misinterpret shaped YAML fragments as
extension dependency references.

Signed-off-by: Javier Tia <javier@peridio.com>
There is currently no entry point for Container Dev Mode in the CLI.
Without the command tree in place, the subcommand hierarchy, --help
output, and shell completion cannot be exercised or validated before
the underlying orchestration logic (host-side registry, engine-driver
watcher, device bootstrap) is built.

Introduce the `avocado container dev` subcommand family with five
verbs: `up`, `sync`, `status`, `down`, and `prune`. Each handler
returns a not-yet-implemented error at this stage. This establishes
the full dispatch path through the clap enum hierarchy so that
integration points and help text can be reviewed independently of
the host-side implementation work that follows.

Signed-off-by: Javier Tia <javier@peridio.com>
Container Dev Mode needs a local registry to cache OCI blobs and
manifest tags between push/pull cycles. Without a dedicated store,
there is no safe place to persist layer data across operations, and
nothing prevents one project's blobs from polluting another's cache.

Introduce a content-addressed BlobStore rooted at
`~/.avocado/container-dev/<project>/registry/` that stores blobs
under a `blobs/<algorithm>/<hex>` layout matching the OCI image
layout spec. Writes are deduplicated by digest so a blob that is
already present is never stored a second time, and all writes are
atomic via a temp-file-then-rename sequence to avoid partial blobs
on crash. Tag pointers live under `manifests/tags/<tag>` and are
similarly written atomically. Per-project namespacing is enforced
structurally so that GC in one project can never sweep another
project's blobs. Digest and tag inputs are validated to reject
path-separator and traversal sequences before they can influence
filesystem paths.

Signed-off-by: Javier Tia <javier@peridio.com>
Without HTTP handlers exposing the content-addressed store built in
task 3.1, a device engine has no way to pull images from the embedded
registry during a dev-mode iteration. The store exists but is entirely
unreachable over the network until the read half of the OCI Distribution
protocol is implemented.

Introduce the registry read module covering the three endpoints a
container engine exercises on a pull: the v2 base check, manifest
retrieval by tag or digest, and blob retrieval with Range support. Tags
are resolved through the existing BlobStore and manifests are served
with the correct media type read from their stored content, ensuring an
engine can correctly distinguish a single-platform manifest from a
multi-arch image index. Ranged blob fetches return 206 Partial Content
with a Content-Range header, matching the resumable-download behavior
engines rely on for large layers. HEAD requests are handled transparently
by axum's routing without a separate handler. The assembled Router is
exported here but intentionally left unbound; it will be mounted onto
the dedicated bulk read listener in task 3.7.

Signed-off-by: Javier Tia <javier@peridio.com>
Currently the embedded Container Dev Mode registry only serves read
requests (blob/manifest GET and partial-content streaming). There is
no write path, so a container engine cannot push images to the
registry, making the dev-loop push-pull cycle impossible.

Add a host-only write token credential type (HTTP Basic, fixed
username, password equal to the write token) and a separate write
router covering the full OCI push surface: monolithic and chunked
blob upload (POST/PATCH/PUT on `.../blobs/uploads/...`), manifest
PUT, and the push-side blob dedup HEAD. The write router is gated
entirely behind a middleware that validates the Basic credential and
rejects anything else — including a Bearer read/control token with
the same secret value — before any handler is reached. This enforces
the design constraint that a device, which only ever receives a Bearer
read/control token, cannot reach a write route regardless of topology.
The write router is intentionally kept on a distinct router object to
be bound onto a separate listener by later tasks (3.6/3.7); it is
never merged onto the bulk read listener.

Signed-off-by: Javier Tia <javier@peridio.com>
Previously the auth module only implemented the write-side Basic
validator. The read listener and the future control-WebSocket upgrade
(task 5.1) had no auth surface defined, meaning any future binding of
the read routes would have been open or would have required a separate,
independently implemented credential check — risking the two surfaces
diverging (design concern G-5).

Introduce a single authorization seam, `read_request_authorized`, that
both the bulk read listener middleware (`require_bearer_read`) and the
control-WS upgrade handler will call. Because both surfaces funnel
through the same predicate the WS upgrade cannot accidentally implement
a looser or different check. The validator enforces scheme separation
(M-2): a Basic write credential is rejected on a read route by
construction, since `Bearer` and `Basic` are distinct schemes. The
`read_router` public constructor is updated to accept a `ReadToken` and
apply the gate, while the internal `read_routes` assembly remains
ungated so existing handler-semantics tests are not burdened with auth
noise.

Signed-off-by: Javier Tia <javier@peridio.com>
Without a sweep path, the per-project blob store accumulates layers
indefinitely. Orphaned blobs left behind by retagged or abandoned pushes
are never reclaimed, growing disk usage without bound. There is also no
protection against a `prune` command racing with a device actively
pulling layers, which could sweep a blob the pull still needs and leave
the device with a broken image.

Introduce an explicit GC path (`collect_garbage`) that walks all
currently-tagged manifests, follows their references transitively
(including multi-arch indices to their sub-manifests), and removes any
blob on disk that is not reachable from a live tag. GC is deliberately
invoked only from `prune` and `down`, never implicitly during a push or
on a timer, to keep the sweep boundary predictable. To guard against the
mid-pull race, a reference-counted `PullGuard` RAII type is added;
`begin_pull` increments a shared atomic counter and the guard decrements
it on drop. `prune` checks the counter and returns `PruneWhilePulling`
if any pull is in flight, while `collect_garbage` (used by `down`, which
tears down listeners first) bypasses that check unconditionally.

Signed-off-by: Javier Tia <javier@peridio.com>
Container Dev Mode needs mutually-authenticated TLS between the host
registry and the device, but previously had no mechanism to generate
per-project certificates or session tokens. Without this, the bulk-read
and control-WS listeners have no credential material to bind against,
and the bootstrap payload delivered to the device has nothing to pin
the host with.

Introduce the tls module (task 3.6) which mints, in a single operation,
a per-project CA and a CA-signed server leaf whose SANs cover the
runtime name, the QEMU guest-to-host alias (10.0.2.2), and the loopback
address. The notBefore is backdated to year 2000 so that RTC-less
devices that cold-boot at the Unix epoch or their firmware build date
still fall inside the validity window. The CA private key is consumed
during leaf signing and deliberately never stored as a struct field,
making it impossible for it to reach any serialized payload. The device
receives only the CA certificate and the read/control Bearer token via
BootstrapPayload; the host-only Basic write token and the CA key are
structurally excluded from that type. Both tokens carry 256 bits of
randomness and rotate independently on every mint.

Signed-off-by: Javier Tia <javier@peridio.com>
The OCI read router previously had no dedicated bound socket, meaning
bulk blob transfers had no guaranteed separation from the control
WebSocket channel. Without this separation, a large layer pull could
head-of-line-block control frames on the same byte stream, violating
the three-listener isolation model required by the session design.

Introduce BulkListener, a self-contained handle that binds a TCP socket,
wraps it with TLS termination using the per-session leaf certificate, and
serves the token-gated OCI read router exclusively on that socket. The
TLS layer is handled by a custom axum Listener implementation that
absorbs transient accept errors with a brief back-off and silently drops
failed handshakes, keeping the server loop running without surfacing
per-connection noise. Because bulk reads live on their own socket,
devices handed only this endpoint cannot reach the write listener, and no
blob body can arrive as a WebSocket frame. Dropping the BulkListener
handle aborts the backing task, so the socket lifetime is tied directly
to the session lifecycle.

Signed-off-by: Javier Tia <javier@peridio.com>
The Container Dev Mode watcher needs to observe the host container
engine for image tag events so it can trigger layer sync to the device
on rebuild. Without a well-defined engine abstraction, both the event
stream logic and credential injection would need to be duplicated or
entangled across Docker and Podman code paths.

Introduce an EngineDriver trait that captures everything engine-specific
about watching for tag events and injecting push credentials. Two
concrete drivers are provided: DockerDriver, which drives `docker events
--format {{json .}}` and injects credentials via an ephemeral
DOCKER_CONFIG directory, and PodmanDriver, which drives `podman events
--format json` and injects credentials via per-invocation `--creds`.
Both drivers communicate exclusively through the engine CLI subprocess,
never through the API socket, so a rootless Podman installation without
a running `podman.socket` works correctly. The engine-agnostic
`forward_tag_events` and `watch_tag_events` helpers drive whichever
engine driver they receive, meaning the watcher orchestration and push
wiring added in subsequent tasks reuse this plumbing unchanged.

Signed-off-by: Javier Tia <javier@peridio.com>
Currently there is no mechanism to detect image rebuild events, select
the correct transfer strategy for the host topology, and notify the
device after layers are synced. Without this, the container dev-mode
loop has no way to react to a `docker build` or `podman build` and
propagate the result to the embedded registry on the device.

Introduce the watcher module that consumes tag events from the engine
driver, debounces rapid rebuilds to a single sync of the latest tag,
and supersedes any in-flight transfer when a newer event arrives. The
sync strategy is chosen by explicit host-topology detection rather than
emergent behavior: native Linux and the avocado-vm take a delta PUSH
into the embedded registry, while Docker Desktop and podman-machine
without the VM take a full-image INGEST export as the only reachable
fallback. The notifier and syncer are expressed as seams so the control
WebSocket (task 5.1) and the concrete engine transfer can be wired in
later without coupling this module to either.

Signed-off-by: Javier Tia <javier@peridio.com>
Syncing a container image built for one CPU architecture to a device
running a different architecture results in a silent wrong-arch
delivery. The container engine on the device may fail to run the image,
or in the case of a multi-arch manifest, pull silently but never
execute correctly. There was no check to catch this before the image
was pushed or the device was notified.

Introduce an architecture guard decorator that sits in the sync path
and probes the image's platform architecture before delegating to the
real syncer. The guard compares the image's canonical GOARCH
architecture against every connected device's reported architecture,
sourced from their `hello` control frames. A single mismatched device
refuses the entire sync with an actionable error that names the
device's target platform and provides the correct `docker buildx build
--platform` invocation. Because the guard returns an error before the
wrapped syncer runs, neither the push nor the device notification is
ever triggered for a mismatched image. An arch normalization layer
reconciles `uname -m` spellings (e.g. `aarch64`, `x86_64`) with
GOARCH spellings (e.g. `arm64`, `amd64`) so comparisons are reliable
regardless of which convention the source uses.

Signed-off-by: Javier Tia <javier@peridio.com>
Without a control channel, the host has no way to push image availability
notifications to connected devices or reconcile a device that reconnects
with a stale running digest. Bulk transfers are handled by a dedicated
HTTPS listener, but there is no signalling path to tell a device when
and what to pull, nor to detect that a device came back out of sync.

Introduce a control-only WebSocket server that exchanges lightweight
control frames between host and device. The host sends a single Sync
frame carrying image coordinates and a digest reference; the device
responds with Hello, Progress, and Status frames. Blob bytes never
travel this channel by construction: HostFrame has no variant that
could carry them, making an accidental bulk transfer a compile-time
error rather than a runtime constraint.

Two invariants are load-bearing. Desired state is re-derived entirely
from the engine's current watched tags at every up, with no disk-restore
path, so a digest that changed while the host was down is always
reflected rather than silently restored from a stale snapshot. On each
device reconnect the host compares the reported running digest against
the desired state and issues reconcile Sync frames for every entry that
does not match, driving the device back to current without manual
intervention.

Authentication reuses the shared read/control-token validator that the
bulk listener's middleware already calls. The WebSocket upgrade is an
HTTP GET carrying the same Authorization header, so the upgrade callback
delegates directly to that function rather than implementing a separate
auth surface. ControlServer also implements the watcher's Notifier seam,
broadcasting a Sync frame to every connected device when the watcher
reports a new tag and updating the desired state so later reconciles
compare against the freshly pushed digest.

Signed-off-by: Javier Tia <javier@peridio.com>
Previously the `avocado container dev up`, `down`, and `status`
subcommands were unimplemented stubs that returned a
not-yet-implemented error. The command tree, `--help`, and completion
wiring could be exercised, but no actual dev session could be started,
monitored, or stopped. This left the Container Dev Mode feature
entirely inoperable.

Implement the full `up`/`down`/`status` lifecycle. `up` mints fresh
TLS material and both session tokens, binds a dedicated bulk read
listener on all interfaces and a loopback-only write listener on a
separate port, starts the engine-driver watcher and control WebSocket,
auto-detects the reachable host IP against the target device (with
`AVOCADO_CONTAINER_DEV_HOST`/`PORT` overrides), delivers the bootstrap
payload once over SSH, and then runs in the foreground until SIGINT or
SIGTERM. `down` signals the foreground `up` process to shut down via
SIGTERM and clears the session state file. `status` reads that same
state file and surfaces a re-bootstrap warning when any device has
presented a stale token.

The accompanying `bootstrap` module provides the testable primitives
these commands depend on. `DeviceBootstrap` is structurally constrained
to carry only the bulk endpoint, the read/control token, and the CA
certificate — there is no field for the write token or write-listener
address, so neither can ever appear in a serialized payload delivered
to a device. `WriteListenerGuard` runs its teardown closure from
`Drop`, ensuring the routable write listener is torn down on any exit
path, clean or unclean. `TokenRegistry` keeps a rotated-out read token
valid until its in-flight bulk connections drain to zero or a hard
ceiling elapses, rather than using a fixed timer that would issue a
mid-stream 401 to an in-flight image pull on a slow link. Stale tokens
surface as `TokenStatus::NeedsReBootstrap` in `DevStatus` rather than
silently retrying.

Signed-off-by: Javier Tia <javier@peridio.com>
Previously, `container dev sync` and `container dev prune` both exited
immediately with a "not implemented yet" error. Users had no way to
manually trigger a one-shot re-push of a watched image to a connected
device, nor any way to reclaim disk space used by unreferenced blobs in
the per-project store.

Implement `sync` as a signal-based trigger: a separate `sync` invocation
sends SIGUSR1 to the running `up` process, which drives one pass of the
same push-then-notify pipeline the file watcher uses on every rebuild.
This reuses the live session's registry write listener, engine syncer,
and control WebSocket, so the notification reaches the device without
requiring additional SSH connections. If no active `up` session exists,
`sync` reports this clearly rather than silently doing nothing. A failed
re-push surfaces as an error and suppresses the device notification,
preserving the invariant that a device is never told an image is ready
when the push did not land.

Implement `prune` as a thin wrapper over the existing per-project store
GC policy: it sweeps blobs unreferenced by any currently-tagged manifest,
refuses to run while a device is mid-pull to avoid sweeping a blob a
pull still needs, and deliberately leaves the session token and CA
material untouched since those live outside the store's registry tree.

Signed-off-by: Javier Tia <javier@peridio.com>
The control WebSocket accepted plain TCP and ran the WebSocket upgrade
directly on the raw stream, so the host<->device control channel carried
sync frames and the reconcile handshake in cleartext. The design binds
the control channel to the same per-project pinned-CA TLS the bulk
listener uses (D8/D9), and the device agent dials wss:// pinning the
session CA -- against a plaintext listener that connection cannot
complete and the pinned-CA guarantee does not hold.

Make the connection-handling core generic over the transport and add a
serve_tls entry that handshakes each accepted stream with the session
leaf before the upgrade, mirroring the bulk listener's TlsListener. The
plain-TCP serve entry is gated cfg(test) so no production path can bind
the control WS in cleartext, and the shared read/control-token validator
seam is preserved rather than duplicated per transport.

Signed-off-by: Javier Tia <javier@peridio.com>
The container dev mode registry and control WebSocket expose two
distinct authentication surfaces: a Bearer-gated TLS bulk read
listener and a Basic-gated plain-HTTP write listener. Without
interface-level tests exercising the real listeners, regressions
that accidentally serve unauthenticated requests or accept the
wrong credential type on the wrong interface would go undetected
until runtime.

Add a dedicated integration test file that spins up live listeners
from a real DevSession and drives them with a pinned-CA client,
asserting that each auth gate rejects exactly the requests it must
reject. The suite covers five failure modes: unauthenticated reads
and WS upgrades being served, unauthenticated writes succeeding on
either interface, the Bearer read token being honored on write
routes (compromised-device scenario), a wrong-password Basic
credential being accepted on a write route, and the Basic write
token being accepted on a read route. Each test is written as a
falsifier so that any gate removal turns a passing assertion into a
failure.

Signed-off-by: Javier Tia <javier@peridio.com>
Without integration-level coverage, the arch guard introduced in task
4.3 could regress silently: a future change might accidentally allow a
mismatched-arch image to pass through to a device that cannot run it,
and no test would catch it. Unit tests on individual functions are
insufficient because they do not verify that the guard, the device-arch
book, and the syncer compose correctly under realistic call patterns.

Add a dedicated integration test file that exercises the real guard
types (`ArchGuardSyncer`, `HelloArchBook`, `check_arch`, `ArchMismatch`,
`DeviceArch`) end-to-end using only two narrow test doubles: a fixed
`ImageArchProbe` standing in for an engine `image inspect`, and a
`ShipRecorder` that counts actual sync calls. This pairing makes
refusals and passes concretely observable: a refusal is proven by an
`ArchMismatch` error AND a ship count of zero, while a pass is proven
by a ship count of exactly one. The suite covers the full decision
matrix: single-device mismatch, single-device match, mixed fleet where
one mismatched device refuses the whole sync, homogeneous matching
fleet, and the canonical `uname`/GOARCH spelling equivalence between
`aarch64` and `arm64` that must not trigger a spurious refusal.

Signed-off-by: Javier Tia <javier@peridio.com>
Previously the control WebSocket listener was bound on an ephemeral
port (`0.0.0.0:0`), meaning the assigned port was unknown at bootstrap
time. The device agent receives its connection parameters once, at
bootstrap delivery, so a port that is only discovered after binding
can never be communicated to the device. This made the control channel
unreachable in practice.

Bind the control WS on a fixed, configurable port (default 5600,
overridable via `AVOCADO_CONTAINER_DEV_WS_PORT`) and include its
device-reachable address as a first-class `ws_endpoint` field in
`DeviceBootstrap`. The endpoint is resolved using the same
device-reachable host as the bulk listener, keeping the two endpoints
symmetric and consistent with the existing host-resolution logic. The
control-WS listener remains structurally distinct from both the bulk
read listener and the write listener; the write-listener address is
still never disclosed to a device, and the bootstrap payload still
carries no field for it.

Signed-off-by: Javier Tia <javier@peridio.com>
The container dev mode write listener served plain HTTP, which worked
for native Linux because Docker's built-in loopback exemption
(127.0.0.0/8) allows insecure connections. However, when the container
engine runs inside an avocado-vm guest, it reaches the host listener
through the QEMU SLIRP alias 10.0.2.2, which is not inside Docker's
trusted loopback range. The guest daemon therefore requires HTTPS, and
before this change any push from an avocado-vm engine would fail at
the transport layer.

A second problem existed even when the transport was corrected: axum's
default 2 MiB body limit caused any real image layer upload to be
rejected with 413 mid-stream, making the write path unusable for
non-trivial images regardless of topology.

Introduce topology detection that selects either the native loopback
plain-HTTP path or the avocado-vm authenticated HTTPS path at `up`
time. On the VM path the write listener terminates the same per-project
leaf TLS as the bulk and control listeners, bound to a known port so
the guest's certs.d trust directory and the pushed image tag can both
be keyed on the identical 10.0.2.2:<port> address. The per-project CA
is delivered into the guest engine's docker trust store over SSH at
`up`, never baked into the VM overlay, satisfying the per-connection
delivery requirement. The write path is authenticated with a host-only
Basic write token; the device-delivered read token is never accepted on
a write route. Lift the body limit on the write router so large image
layers are accepted. Add a verification script and tests covering each
falsifiable property of the VM write path.
Without integration-level tests, the two core properties of the
container dev mode sync loop have no falsifiable coverage: that a
one-line change transfers only the changed layer (not the whole image),
and that a device reporting a stale digest receives a sync frame over
the control WebSocket prompting it to pull and restart.

Add an end-to-end test suite that exercises both properties against the
real listeners a device talks to in production. The delta-pull test
drives the write listener and bulk TLS listener over a shared
content-addressed store, verifying that byte-identical blobs are never
re-transferred after a top-layer change. The control-WS test dials the
TLS-upgraded WebSocket with a pinned session CA and a Bearer token,
sends a Hello carrying the stale running digest, and asserts that the
host responds with a sync frame carrying the new digest. Together these
tests catch regressions in the store deduplication logic and the
reconciliation path without requiring a real device or container runtime.

Signed-off-by: Javier Tia <javier@peridio.com>
The authenticated VM write path (task 7.1) was validated against a local
QEMU engine VM, but only the verify script was committed - the harness
that provisions that VM lived as untracked local scratch, so the
end-to-end path could not be reproduced from a clean checkout.

Commit the provisioner (idempotent Debian 12 + docker + ssh engine VM
under QEMU SLIRP, guest reachable at 10.0.2.2 like the macOS avocado-vm),
its example container_dev config, and a README. Generated state (the
qcow2 overlay, ssh key, cloud-init seed, env.sh) is kept in a work dir
outside the repo so a ~900 MB overlay never lands in git; only the base
image download is a manual one-time step.

Signed-off-by: Javier Tia <javier@peridio.com>

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review (cold read of my own +10k PR). The security core is clean: TLS drops the CA key (bootstrap carries only CA cert + read token), tokens are 256-bit CSPRNG, the store closes digest/tag path-traversal, every docker/podman call is argv-only (no shell), and the bind addresses match the threat model. Six correctness/robustness findings inline - the two worth acting on first are the recycled-PID signal hazard in down/sync and the cross-arch guard never being wired into the live up path (confirm whether that's intentionally deferred - if so it shouldn't read as complete).

Comment thread src/commands/container/dev.rs
Comment thread src/commands/container/dev.rs Outdated
Comment thread src/commands/container/dev.rs
Comment thread src/utils/container_dev/registry.rs
Comment thread src/utils/container_dev/registry.rs
Comment thread src/commands/container/dev.rs Outdated
jetm added 5 commits July 27, 2026 18:10
Several comments and WriteListenerGuard's own docs describe `down` tearing
down "the routable write listener and its 0.0.0.0 forward" and guarding
against an "authenticated LAN write port". No such bind exists: the write
listener is created on 127.0.0.1 and the VM push path reaches it through
QEMU's 10.0.2.2 host alias rather than a routable address.

The code is safer than its description, which is the problem. A reviewer
reading these comments audits the guard as the only thing standing between
an unclean exit and a LAN-exposed authenticated registry, and weighs its
correctness against a threat that is not there - while the real reason the
guard earns its keep, not stranding a bound port for the next `up`, goes
unstated. Describe the loopback bind the code actually makes.

Signed-off-by: Javier Tia <javier@peridio.com>
`up` handed the bare EngineSyncer to both run_watcher and the SIGUSR1 sync
trigger, so ArchGuardSyncer and EngineArchProbe existed only in their own
unit tests and in tests/container_dev_arch.rs. The ControlServer was already
recording every device's hello.arch into a HelloArchBook, and nothing ever
read it. An amd64 host targeting an aarch64 device therefore built, pushed
and notified an amd64 image - the silent wrong-arch delivery the guard was
written to refuse - while every test covering that guard passed.

Wrap the syncer in the guard before either consumer takes it, and share one
arch book between the control server that fills it and the guard that reads
it. Both sync entry points go through the wrapper: the manual trigger can
ship a wrong-arch image just as easily as the watcher, so run_sync_trigger
now takes Arc<dyn Syncer> rather than the concrete EngineSyncer that was
pinning it to the unguarded path.

The probe rebuilds its driver handle because `driver` is moved into the
event watcher earlier in `up`; it retains only the engine binary name, so a
second handle costs nothing.

Signed-off-by: Javier Tia <javier@peridio.com>
`up` removes session.json only on the graceful path, so a panic or a
SIGKILL leaves the file behind carrying a pid that is no longer `up`.
`down` then SIGTERMs that pid and `sync` sends it SIGUSR1, neither checking
whether the process is still the one that wrote the file. Pids get recycled:
when an unrelated process inherits the number, `sync` delivers SIGUSR1 to it
and kills it outright, since SIGUSR1 terminates by default. The comment
justifying the unchecked kill argues a stale pid yields ESRCH, which holds
only while the pid stays unused - exactly the case that is not the hazard.

Checking liveness by pid cannot fix this, because a recycled pid is live.
Take an flock on the session file for the lifetime of `up` instead: the
kernel releases it however the holder dies, including under SIGKILL, so
being able to acquire it proves no `up` owns the file - something a stale
record cannot fake. `down`/`sync`/`status` test it before trusting the pid
and clear the record when it turns out to be abandoned.

Two things fall out of the same lock. A second `up` on one project is now
refused instead of racing the first one's listeners, and `status` reports a
dead session as "not running" rather than replaying registry_running=true
for listeners that died with the process.

Signed-off-by: Javier Tia <javier@peridio.com>
The module docs described `status` as surfacing stale-token re-bootstrap
state through the drain-based TokenRegistry, which reads as a shipped
feature. Neither half is reachable. `up` writes session.json once and never
revisits it, so `status.devices` stays the empty vec it was constructed
with and `needs_rebootstrap()` is structurally false rather than merely
usually false. Rotation cannot cross an `up` at all: `TokenRegistry::rotate`
takes `&mut self`, and a re-`up` is a new process starting from a fresh
registry, so there is no rotated-out token for a later invocation to
classify.

Both are implemented and unit-tested in bootstrap.rs, which is what makes
the docs plausible and the gap easy to miss on review. Saying so - and
naming what it would actually take, `up` publishing session state as it
runs - keeps the next reader from auditing a path that cannot execute, or
from assuming a stale device would be caught here.

Signed-off-by: Javier Tia <javier@peridio.com>
Two ways the write listener let one push hold more host memory than it
needs to.

Abandoned upload sessions were never reclaimed. A POST opens a session and
PATCHes buffer into it, but nothing obliges a client to send the finalizing
PUT - an interrupted push or a killed `docker` simply leaves its buffer in
the map. Across a long `up` session, repeated interrupted pushes grow the
host process without bound, and no client ever announces that it gave up.
Evict sessions untouched past a TTL when a new one opens, which is both the
moment another buffer is about to be allocated and the only point an
abandoned one can be noticed. The clock advances on every PATCH, so a slow
link is judged on the gap between chunks rather than total transfer time
and is never evicted mid-push.

The HEAD dedup probe read whole blobs to report their length. `docker push`
HEADs every layer before uploading, and the store exposed no size path, so
each probe pulled a full existing layer into RAM and dropped it - putting
the already-present half of an image on the heap to answer "do you have
this?". `blob_size` stats the entry instead.

The body limit moves from disabled to 2 GiB rather than staying off. The
2 MiB default does 413 a real layer, which is why it was disabled, but
bodies are buffered whole before any handler runs, so no limit means one
request can allocate without bound. 2 GiB clears any dev-loop layer while
still capping a single request.

Signed-off-by: Javier Tia <javier@peridio.com>

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent second review of the increment since f69b3e2 (+444/-55, 4 files), traced cold at 2295751. Ten findings inline, two of them high.

The two I would fix before this merges are both about a guard that does not guard:

  • dev.rs:365 - the arch guard this diff wires up is fail-open while the arch book is empty, and reconcile never consults the book at all. Since up spawns the watcher before it bootstraps the device, the empty-book window is the normal case, not an edge: a rebuild pushed before the device ever connects gets recorded into desired state and delivered verbatim on the device's first frame, architecture unchecked. The comment right below claims this decorator prevents exactly that.
  • dev.rs:436 - SessionLock::acquire runs after the state write, the token mint, all three binds, the watcher spawn, and both SSH deliveries, so it cannot exclude a second up from any of them. Two of its consequences need no unusual configuration at all: a concurrent status/sync/down in the two-statement window makes acquire fail ENOENT (it lacks .create(true)) after everything is already bound and the device is bootstrapped, and because every read path unlinks the lock inode, mutual exclusion does not survive a single down.

Two findings are backed by running the code rather than reading it. Deleting session.touched = Instant::now(); (registry.rs:407) leaves the test suite at 10/10 passing, so patching_a_session_keeps_it_alive_past_the_ttl asserts nothing about the TTL - it trips the workspace CLAUDE.md rule that every test must fail when the implementation is wrong. And the default-config second-up really does die at EADDRINUSE first (mio sets only SO_REUSEADDR), which is why the destructive half of the lock finding is scoped to the two env overrides rather than claimed outright.

One doc contradiction worth fixing on its own: registry.rs lines 62-63 say only the gap between chunks counts against the TTL, while lines 70-72 say bodies are buffered as Bytes before anything inspects them. The second is what the code does, so the first is wrong, and the commit body repeats it as an absolute ("never evicted mid-push").

Traced and cleared, so nobody re-audits: HelloArchBook's derived Clone does share one map (the Arc is cloned, not the BTreeMap), so the book genuinely is shared between ControlServer and the guard - that was my main suspicion and it is fine. The Arc<dyn DeviceArchBook> double-wrap is the dyn coercion, not redundancy. EngineArchProbe::new(...as_ref()) on a temporary Box is sound because new copies out a &'static str. A refusal from the guard does make do_sync_and_notify skip the notify, as its comment claims. require_basic_write wraps the whole router, so an anonymous request causes no large allocation despite the body limit. The loopback-only doc rewrites are accurate in both directions. read_blob to blob_size is behavior-preserving and the digest is still validated. And I dropped one candidate outright: the new test's Instant::now() - TTL - 1s does not panic on a freshly booted host, since Linux Instant is a signed-seconds Timespec - verified on a box with 3,700s uptime.

Scope note: this was the requested light-to-moderate pass - three finder angles plus four verifiers, no separate gap sweep. avocado-cli has no repo-level CLAUDE.md, so the workspace one governs.

Comment thread src/commands/container/dev.rs
Comment thread src/commands/container/dev.rs Outdated
Comment thread src/utils/container_dev/registry.rs Outdated
Comment thread src/commands/container/dev.rs Outdated
Comment thread src/utils/container_dev/registry.rs
Comment thread tests/container_dev_arch.rs
Comment thread src/utils/container_dev/watcher.rs
Comment thread src/utils/container_dev/registry.rs Outdated
Comment thread src/commands/container/dev.rs
Comment thread src/commands/container/dev.rs Outdated
jetm added 7 commits July 28, 2026 15:46
The lock was acquired at the very end of `up`, after minting tokens, binding
all three listeners, spawning the watcher and SSHing a fresh bootstrap over
the device's existing one. A second `up` therefore did all of that before
discovering it had lost: it repointed the device at a listener the winner
will 401, truncated the winner's state file to write its own pid, and only
then bailed - and because `?` propagates, the teardown that clears the file
never ran. `down` would then SIGTERM a dead pid and report success while the
first session kept its authenticated write listener bound, invisible to
`status`. A lock taken after the work it guards only reports collisions that
already happened.

Acquire it first, before anything observable.

The lock also could not survive its own teardown. It was held on
`session.json`, which every teardown path unlinks, so the next `up` locked a
freshly created inode and mutual exclusion silently lapsed after a single
`down`. Move it to a dedicated `session.lock` that is created once and never
removed, so the contended inode is stable for the life of the project dir.
`acquire` gains `create(true)` because it now runs before any state exists -
without it the first `up` in a project would fail ENOENT, as would any `up`
racing a concurrent reader that had just cleared the file.

The liveness probe drops to `LOCK_SH` on a read-only handle. It answers a
question and should not perturb what it observes: an exclusive probe made a
concurrent `up` fail with the misleading "another `up` is already running"
if a `status` held the lock for that instant, serialized two concurrent
probes against each other, and turned an unwritable or read-only-mounted
`session.json` into a hard error out of commands that used to report fine.

Fold the triplicated stale-check into `load_live_session` so the rule for
whether a recorded pid is safe to signal has one home instead of three
copies to keep in agreement.

Signed-off-by: Javier Tia <javier@peridio.com>
`patching_a_session_keeps_it_alive_past_the_ttl` asserted nothing. Its sweep
ran milliseconds after the POST, so `now - touched < 600s` held whether or
not the refresh existed: deleting `session.touched = Instant::now()` left the
whole suite green. The one guard against the eviction-race class was
decorative, and the test's name claimed a property it never exercised.

`evict_expired` now takes `now` instead of reading the clock, and the write
router can be built over a caller-supplied session map, so a test can place a
live session at a chosen distance from the boundary. The rewritten test
backdates a session past the TTL, PATCHes it, then triggers a sweep - it
survives only if the handler really rewrote `touched`. Deleting that line now
fails the test. Two more pin the other directions: the boundary itself one
tick either side, and an un-patched session being swept with its next chunk
404ing rather than silently succeeding.

Also correct the TTL comment, which was wrong in a way the commit that added
it repeated as an absolute. It claimed only the gap between chunks counts,
never total transfer time, while the body-limit comment eleven lines away
said bodies are buffered whole before any handler runs - and both cannot be
true. `touched` advances after a chunk is buffered, so a layer sent as one
large PATCH keeps its timestamp pinned for the entire transfer and a
concurrent POST's sweep can evict it mid-flight. The comment now states that,
along with the bound that makes it unlikely (a single request slower than
600s, about 28 Mbit/s for a 2 GiB layer) and what closing it would actually
take - marking the session in-flight at request receipt, which is middleware
rather than a constant.

Signed-off-by: Javier Tia <javier@peridio.com>
`run_sync_trigger` built its `TagEvent` with `image_id: None`, which `notify`
turns into an empty digest via `unwrap_or_default` and records as the desired
state. The empty string is not inert there: `reconcile` filters on
`digest != hello.running_digest`, and a device that has not pulled anything
reports an empty `running_digest` - so an empty desired digest compares EQUAL,
emits no Sync frame, and the device is silently never told to pull. Running
`container dev sync` before the device connects was enough to reach it, since
production desired state starts empty and the manual sync was then its only
populator. Every engine-sourced event carries an id, so this was the one
production path that could plant the empty case.

A signal carries no event, which is why this path had nothing to read an id
from. Ask the engine instead: `resolve_image_id` shells out to
`image inspect --format {{.Id}}`, mirroring how the arch probe gets platform.
An image the engine does not know is now a warning and a skip rather than a
digest-less notify.

`notify` also refuses a digest-less event outright. Fixing only the caller
leaves the trap armed for the next one, and the failure it produces is
silence on the device rather than an error anyone would trace back here.

Signed-off-by: Javier Tia <javier@peridio.com>
…h book

Every existing arch test hands `ArchGuardSyncer` a book it populated by hand,
so all of them pass with the two halves disconnected - which is precisely the
state the guard sat in before it was wired into `up`, and precisely what the
wiring commit needed a test for and did not get. Unwiring the guard, or making
`HelloArchBook::clone` a deep clone instead of sharing the `Arc`, left the
whole suite green.

Drive the real path instead: a device sends a `Hello` over the control WS, the
server records its arch, and a guard holding only a clone of that book refuses
an image it never saw recorded. Verified it catches the named regression -
replacing the derived `Clone` with a deep clone fails this test with "the two
halves are not sharing one map" while the rest of the suite stays green.

The pre-hello sync in the same test documents the fail-open window rather than
endorsing it: with an empty book the guard has nobody to disagree with and
ships. That gap is real and is being tracked separately; pinning it here means
a change in that behaviour shows up as a test diff instead of passing quietly.

Signed-off-by: Javier Tia <javier@peridio.com>
The cross-arch guard could only compare against devices connected at push
time, and `up` spawns the watcher before it delivers the bootstrap - so a
rebuild reaching the guard with an empty device book had nobody to disagree
with, was allowed, and recorded its digest as desired. `reconcile` then handed
that digest to the first device to say hello, without ever reading the arch
book it had just written. An amd64 host targeting an aarch64 device shipped an
unrunnable image, which is the delivery the guard exists to refuse.

The obstacle was that the image's architecture was known only inside the
guard, for the duration of one call. `reconcile` runs later and had nothing to
compare a device against. So record it: `ImageArchBook` mirrors `HelloArchBook`
in the other direction - the guard writes what it probed, `notify` reads it and
stores it beside the digest, and `reconcile` filters on the pair.

Putting the arch next to the digest is what makes the invariant structural
rather than timing-dependent. The guard still refuses what it can at push time;
this covers the window where it cannot know yet, at the one moment the device's
own architecture is finally on the wire.

An entry with no recorded arch reconciles unchanged. Entries derived from the
engine's watched tags at `up` never went through the guard, so treating unknown
as mismatch would stop reconciling them entirely - the filter refuses only what
it positively knows is wrong.

Signed-off-by: Javier Tia <javier@peridio.com>
`record_hello` only ever inserted, and nothing removed. Because `check_arch`
refuses on ANY mismatch, one departed device poisoned the rest of the session:
unplug an aarch64 board, attach an amd64 one, and every sync is refused on
behalf of hardware that is gone - with buildx guidance naming an architecture
nothing connected reports. Only restarting `up` cleared it.

The book is trying to answer "what is connected right now", and the connection
set already knows that, so derive the entry from the connection instead of
accumulating it. `record_session` returns a lease held by the per-connection
task for the life of the session.

A lease rather than a removal call on disconnect, because the failure mode of a
missed removal is the bug itself coming back by another route - a phantom
device refusing syncs with no connection behind it. Drop runs on every exit
from `run_session`: clean close, send error, unwind. There is no path that
forgets.

Leases refcount per device so two overlapping connections from one device
cannot evict each other. Without that a reconnect that briefly overlaps its own
previous session would have the older guard drop the newer session's entry,
silently disarming the guard for a device that is still attached.

`record_hello` stays for callers that populate a book by hand, with its
lifetime hazard documented on it.

Signed-off-by: Javier Tia <javier@peridio.com>
The write path buffered every blob body as `Bytes`/`Vec<u8>` before any
handler saw it, so a body limit was doing double duty: bounding host memory
AND, unavoidably, capping layer size. The 2 GiB value I picked for it was a
regression - the engine sends one layer per request, so a per-request cap is a
per-layer cap, and a layer above it 413'd with a bare axum rejection carrying
no OCI error the engine could act on.

Removing the conflict rather than retuning it. `BlobUpload` writes straight to
a temp file under the store and hashes incrementally, so peak memory is a chunk
rather than a layer and there is nothing left for a size limit to protect.
Digest verification is unchanged in substance - still computed over what was
actually written, never trusted from the client - only where the bytes lived
while it happened. An upload dropped without `finish` takes its temp file with
it, so an abandoned push leaves no residue.

The DefaultBodyLimit layer is gone rather than set to `disable()`. It would be
inert: that limit is consumed by the `Bytes` and `String` extractors, and every
write handler now takes `Body`, so the layer would gate nothing while reading
as though it still did. Manifest PUT is the one path that must have the whole
document to parse it, and it applies an explicit 32 MiB bound at the read.

`patch_route` and the finalizing `put_route` take the session out of the map
for the duration of the transfer. The mutex cannot be held across the await,
and a session actively being streamed into is not one a concurrent sweep should
be able to reclaim - removing it makes both true without a second lock.

On coverage: the >2 GiB single request that motivated this is not reachable in
a test, and toggling the old limit cannot stand in for it now that the
handlers take `Body`. The absence of a ceiling is a property of the signatures.
What the tests do pin is the chunked path end to end and that the staging file
grows as chunks arrive rather than accumulating in memory.

Signed-off-by: Javier Tia <javier@peridio.com>

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second-reviewer pass on my own PR, cold, on the change since the last round. Twelve findings inline, most severe first. Four of them are the ones I'd land before anything else: the session lock no longer proving pid ownership (1), the arch filter that only covers one of two notify paths (2), the mid-transfer upload session teardown (3), and registry/uploads/ never being swept (4).

Findings 3, 4, 5, 6 and 12 all trace to the same shift in this round: the blob path moved from buffer-in-memory to stream-to-disk. That was the right move, but the invariants that used to fall out of buffering for free - a request either arrived whole or not at all, an abandoned upload died with the process, a blob could only reach disk if it fit in RAM, and a size cap existed - each needed an explicit replacement, and only the manifest path got one.

Findings 9, 10 and 11 are a set: three tests whose doc comments claim a falsifier they do not have. In each case I checked by making the change the comment says would fail, and the suite stayed green. Worth fixing as a group, because together they mean the lock mechanism and up's guard wiring are effectively untested.

Below the cap - confirmed, lower severity

  • store.rs:390 - BlobUpload::finish re-implements blob_path (store.rs:326) inline plus write_blob's exists/create_dir_all/persist tail, so the blob layout now has two authors while only readers go through blob_path. blobs_root is also initialized from self.root and then has "blobs" joined onto it, and format!("sha256:{hex}") hardcodes an algorithm two lines above a parse_digest that accepts any.
  • store.rs:341 - parse_digest's doc comment is orphaned onto pub struct BlobUpload, so cargo doc renders the new public type's summary as "Split an OCI digest into its (algorithm, hex) components", and parse_digest at store.rs:402 ends up undocumented.
  • store.rs:360/:378 - unbuffered write_all per hyper frame means syscall count per GiB is set by link behavior rather than bounded by code; flush() before persist is a documented no-op on File, and with no sync_all a host crash can leave the blob path present while its tail never reached disk, which has_blob and the HEAD dedup probe both report as complete.
  • watcher.rs:601 - record_hello has no production callers left (only watcher.rs:962/990 and six sites in tests/container_dev_arch.rs) and plants holders: 1 with no lease, so its entry can never be released. The struct doc at watcher.rs:579 still says reconnects "overwrite" while record_session six lines below says they refcount.
  • ws.rs:441 - the inner Option<DeviceArchLease> is never None, since the only Some(..) return arm always supplies a lease, so if lease.is_some() is a branch that cannot be taken.
  • ws.rs:1015 - notify_refuses_an_event_with_no_image_id labels frames.is_empty() "The decisive assertion", but an empty desired digest compares equal to the empty running_digest, so that assertion passes with the digest.is_empty() bail deleted. Only the third assertion is decisive. assert!(result.is_err()) also never checks which error.
  • tests/container_dev_arch.rs:323 - pinned_ca_connector is now the fourth byte-identical copy (also container_dev_security.rs:131, container_dev_e2e.rs:353, ws.rs:1064), and tests/common/mod.rs already exists as the shared home.
  • dev.rs:949 + watcher.rs:740 - two engine spawns per image per manual sync, to read two fields of the same image inspect object, awaited serially per image.

Checked and refuted

Recording these so nobody re-derives them. Digest verification and path traversal in the new write path are sound: finish compares against the incrementally computed hash before any filesystem move, the dedup branch is only reachable after the hash matched, and parse_digest's permissive algorithm parsing is unreachable because a non-sha256: digest fails equality first. Auth is not bypassable - require_basic_write is a from_fn_with_state layer over the whole router, applied outside routing, so it precedes the /v2/ ping and unrouted methods too. No listener moved this round. Secret file permissions are fine: session.lock is 0644 but always empty (it exists only as a stable flock inode), and persisted blobs keep NamedTempFile's 0600. No DeviceArchLease::drop self-deadlock and no lock-order cycle between image_arches and desired. Instant subtraction in the test helpers does not panic on Linux, and the Windows job is cargo check only. The missing -- before image in resolve_image_id is real in isolation but byte-identical to the pre-existing EngineArchProbe::image_arch shape this round does not touch.

No rust.md violation: edition 2021, anyhow + thiserror matching the CLI row, and no [lints] table, rustfmt.toml, clippy.toml or workspace introduced.

Comment thread src/commands/container/dev.rs
Comment thread src/utils/container_dev/ws.rs
Comment thread src/utils/container_dev/registry.rs Outdated
Comment thread src/utils/container_dev/store.rs
Comment thread src/utils/container_dev/registry.rs
Comment thread src/commands/container/dev.rs
Comment thread src/utils/container_dev/registry.rs Outdated
Comment thread tests/container_dev_arch.rs Outdated
Comment thread src/commands/container/dev.rs Outdated
Comment thread src/utils/container_dev/registry.rs Outdated
jetm added 4 commits July 29, 2026 10:37
Four defects introduced by the previous round, each a property that used to hold
by accident and stopped holding once the mechanism under it changed.

Publishing the pid immediately after taking the session lock. Moving the lock to
the top of `up` decoupled it from the state file, which reopened the very hazard
the lock exists to close: a predecessor SIGKILLed before teardown leaves its pid
in session.json, and between acquiring the lock and writing the record - minting
TLS, three binds, an SSH round trip - `load_live_session` reported the session
live while handing out a DEAD pid. `sync` would then send SIGUSR1, default
disposition terminate, to whatever recycled that number. The pid is known the
moment the lock is ours, so there is no reason to wait.

Upload sessions now survive a failed chunk stream. Taking the session out of the
map is what stops a concurrent sweep reclaiming it mid-transfer, but returning
early on a stream error dropped the `BlobUpload` - unlinking the staging file and
every chunk already accepted, so a reset on chunk 6 of 8 restarted the layer from
byte 0. Resumability was previously free: the `Bytes` extractor rejected a
truncated body before the handler ran. Streaming has to re-insert explicitly.

`prune` sweeps `uploads/`. `collect_garbage` walks `blobs/` only, so nothing in
the tree ever looked at the staging directory. `NamedTempFile` unlinks on drop,
which covers a clean exit and nothing else - an `up` killed mid-push left its
partial layer on disk permanently while `prune` reported sweeping nothing.

A 32 GiB ceiling on a streamed blob. The buffered path had an accidental one: a
request over the body limit was refused with nothing written. Streaming replaced
it with no bound at all, so a write-token holder could PATCH indefinitely across
sessions and fill the filesystem, and an oversized layer from a bad COPY reached
the same place by accident. Enforced in `BlobUpload::append`, the single funnel
every write passes through, and reported as 413 BLOB_UPLOAD_INVALID rather than a
bare rejection. It bounds the read side too, since `serve_blob` still loads a
blob whole - a blob that cannot be stored cannot later OOM a pull.

Each fix has a test that fails against the pre-fix behaviour, verified by
reverting the mechanism: the session vanishes, the ceiling stops refusing, the
staging file survives prune. The resumability test drives the router through
`oneshot` with a body stream that errors, because a genuinely truncated HTTP
request makes the server wait for bytes that never arrive and hangs rather than
failing.

Signed-off-by: Javier Tia <javier@peridio.com>
Putting the arch check in `reconcile` closed the pre-hello hole from one side
only. `reconcile` runs on a `hello`; `notify` broadcasts to every subscriber and
never goes through it. So a device that connects DURING a push reconciles against
a desired map the push has not written yet, gets no frames, and then receives the
pushed Sync directly on its broadcast arm - unfiltered.

That is the same window, reached from the other side, and it is the likelier one
in practice: the guard snapshots the device book before pushing, and a push of a
large layer is exactly the interval during which a board finishes booting.

The check belongs on the fan-out, and it has to be per-connection because the
broadcast channel carries one frame to all subscribers. Each `run_session` learns
its device's arch from the `hello` it already handles and consults the recorded
image arch before forwarding. `frame_suits_device` refuses only a positive
mismatch: an unprobed image, or a device that has not said hello yet, passes
through unchanged - so this can only withhold a delivery it knows is wrong.

Two tests, the first failing when the filter is disabled.

Signed-off-by: Javier Tia <javier@peridio.com>
…aims

The shared-lock change fixed half of what its own comment claimed. `LOCK_EX`
conflicts with a held `LOCK_SH` exactly as it does with another `LOCK_EX`, so
switching the liveness probe to shared fixed probe-vs-probe and left
probe-vs-`up` untouched: an IDE task polling `status` could land inside the
window and make a legitimate `up` abort with "another `up` is already running"
when none existed, pointing the user at a `down` that would do nothing. Verified
the conflict directly before believing it.

`acquire` now waits out a brief hold rather than failing on the first
EWOULDBLOCK. That separates the two cases on duration instead of guessing: a
probe's hold is over in microseconds, while a competing `up` holds the lock for
its whole lifetime and is still holding it when the window expires.

Three of the four lock tests could not have caught this, because none created
two concurrent holders - they ran sequential probes on one thread and
`session_is_live` releases its flock on every return, so all of them passed with
`LOCK_EX` restored. Replaced with tests that hold a real shared lock across an
`acquire`, and one that asserts the property which actually distinguishes the two
modes: with an exclusive probe, a concurrent reader makes `session_is_live`
report a session live when nothing owns it - a false positive that would have
`down`/`sync` signalling whatever pid the stale record held.

Device-supplied text no longer reaches the terminal raw. `print_warning` is a
bare println with an ANSI prefix, and both `device_id` and `arch` come off the
wire - `DeviceArch::parse` returns the raw lowercased input for anything it does
not recognize. A device holding the read token could put `ESC[2K\\r` and a forged
green success line in its `device_id` and overwrite the refusal warning, so a
refused sync would read on screen as a completed one.

The staging test asserted a counter, not staging. It checked `upload.written()`,
a u64 incremented in `append`, while claiming that proved write-through - so
rewriting `BlobUpload` to accumulate into a `Vec` and only write inside `finish`
left it green, reintroducing the whole-layer-in-memory behaviour this work
exists to remove. It now stats the staging file between chunks and fails against
exactly that implementation. Its second, unrelated assertion is split out.

Two doc corrections, both of which would have misled the next reader rather than
merely being untidy. The arch test claimed unwiring the guard in `up` would fail
it; nothing in the suite touches `DevUpCommand`, so that gap is named as
unclosed instead of claimed as covered. And the TTL comment still described the
buffered path - a `Bytes` extractor that no longer exists, an in-flight session
the sweep can no longer see, and a throughput bound derived from a removed
constant - while prescribing middleware that would duplicate what
remove-on-entry already does. The resource at risk is disk now, not memory.

Signed-off-by: Javier Tia <javier@peridio.com>
…hole

Moving uploads to a streaming write removed the property that a layer had
to fit in host memory before it could reach disk. A chunked push used to
accumulate in a per-session buffer, so an oversized layer failed at push
time and never landed; it now writes straight through, so a 40 GiB layer
stores and tags successfully on a 32 GiB host.

serve_blob still read the whole object back via read_blob, which turned
that into a worse failure than the one it replaced. The first device GET
of such a blob sizes a single allocation by the blob and the OOM killer
takes the `up` process down - with it all three listeners, the watcher,
and the session's minted TLS material. A Range request allocated the
window a second time on top of the full buffer. Unlike the old push-time
failure this one is persistent and repeats on every pull, because the
oversized blob is already stored.

Add BlobStore::open_blob, returning a handle and the size, and serve both
the full body and the range window through ReaderStream. The range case
seeks and takes rather than slicing a buffer, so neither path sizes an
allocation by anything the host chose. Manifests keep using read_blob:
they are capped at 32 MiB and the media-type sniff needs the bytes.

open_blob returns a std::fs::File so the store stays synchronous; the
caller wraps it for the runtime it serves on. The full-body response now
sets Content-Length explicitly, which a streamed body does not get for
free and engines use for pull progress.

tokio-util was already in the tree via axum and tokio-tungstenite, so the
dependency is one new edge in Cargo.lock rather than a new package.

Tested by frame count rather than by allocation: a Body built from one
Vec carries exactly one data frame however large it is, while a streamed
body carries one per read, so >1 frame is the discriminator. Driven
through oneshot because over TCP the chunk boundaries a client sees come
from coalescing, not from how the handler built the body - counting socket
reads would have proven nothing. Both mutations were checked in isolation:
buffering the full path fails only the full-blob test, copying the window
fails only the range test. 1279 lib tests and every integration target
pass; the pre-existing range and suffix-range assertions are unchanged.

Signed-off-by: Javier Tia <javier@peridio.com>

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding, carried over from avocado-os#46 after tracing it to the code that actually owns it. The os#46 side is resolved; this is the residual half, and it partly corrects what I said there.

Comment thread src/commands/container/dev.rs Outdated
The bootstrap file carries the Bearer read/control token, and delivery
wrote it and then corrected the mode:

    ... | base64 -d > path && chmod 0600 path

The redirect creates the file at the remote shell's umask with the token
already written, so it sits on disk world-readable for the width of two
commands - 0644 under a default 0022, measured. Any local user on the
device can read it in that window, and the token is enough to pull from
the session's registry.

Put a umask in force at creation instead, so the mode is right from the
first byte and there is no second step to race. A subshell keeps the
umask change from leaking into anything else run_command may chain later.
This is also less code than what it replaces: one construct rather than
two, with no ordering to get wrong.

Extracted the command into `bootstrap_delivery_command` to make the
property assertable rather than reviewed by eye - the function exists for
the test, and the commit body is the place to say so.

Two tests, and the split between them is deliberate. The behavioral one
runs the generated command under an explicitly permissive parent umask
and stats the result, which pins the outcome. But it passes against the
old shape too, since chmod also ends at 0600 - so it cannot be the guard.
The window itself is unobservable from a test without racing the shell,
so the discriminating assertion is structural: a umask before the
redirect, and no correcting chmod anywhere. Confirmed by reverting to the
old command and watching exactly that test fail while the behavioral one
still passed.

Raised on the review of avocado-os#46, where I had wrongly reported the
token as unprotected; the chmod was already there, and this is the
narrower issue that survived tracing it here.

Signed-off-by: Javier Tia <javier@peridio.com>
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.

1 participant