review: images, CI, docs and the Python SDK - #220
Merged
Merged
Conversation
pty_open spawns $SHELL and falls back to /bin/bash; the android userspace has neither the variable nor bash, so every pty on that flavor failed at spawn. The init service now sets SHELL to /system/bin/sh, the way it already sets SILKD_PORT and the proxy trio. The android smoke only runs exec, so nothing caught it.
The base Dockerfile keys its install-agent RUN on INPUTS_HASH because a secret-mounted script never enters the layer cache; CI computes the hash, make base passed nothing, so a local edit to install-agent.sh silently rebuilt against the cached layer. The target now passes the same content hash CI does. The images target's help line said "all VM images" for two of nine; it now says what it builds.
The amd64 branch asserted VIRTIO_CONSOLE in its arch list and again in the common list. The desktop image fetches Chrome's signed repo over http while the base image had just moved every apt source to https; the repo serves https and the key check is unchanged.
bench.sh runs under pipefail and every other probe on the results step is guarded; the digest pipeline was not, so a host without a cocoon on PATH lost the whole bench at the last step.
release.yml and both Dockerfiles build --locked, the four gate workflows did not, so a Cargo.toml bump without its lock went green in silkd/boot-init and failed in images. publish-pypi installed pytest and ruff unpinned where python.yml pins both.
… cache python.yml ran on every branch push, so a PR push ran it twice; it now runs on main pushes, pull requests and dispatch like its siblings. images.yml, silkd.yml and boot-init.yml did not list their own files (or, for images, the three build workflows it calls), so a change to how an image is built rebuilt nothing. build-os-images hashed every non-Dockerfile input and treated any depth-3 file as cache-busting, so a README edit forced an uncached rebuild of the heaviest image; markdown now stays out of both.
…t out The README promised sandbox.debug and sandbox.trace in the same table that describes the Firecracker lane, but the pinned kernel builds no 8250 UART and cocoon boots that lane with console=ttyS0, so nothing they write is visible there. The three error tables did not say that an unresolvable exec or pty user is a bad_request, nor that the guest now distinguishes an unknown name from a failing name service. The Python errors list omitted SandboxTimeout, which every timeout path raises. The e2e roster listed 17 drivers of 18.
…he whole dial Conn.recv caught OSError only, so a read after close raised the file object's bare ValueError outside the SandboxError hierarchy a Watcher relies on. dial_agent applied its timeout to every read separately — connect, TLS, the status line, each header line — so a node answering one header per interval stretched a 0.2 s dial to as long as it liked; docs/sdk-python.md promises the timeout bounds the dial and upgrade. The dial now derives its own deadline from the timeout (the smaller of that and a caller's deadline) and every step spends from it. A single header line trickled byte by byte is still bounded per read, not per dial; that takes a hostile node. Two tests: recv after close, and headers each arriving under the per-read timeout — the second fails on the previous code.
decode_response slices an exactly shaped data frame and base64-decodes it with validate=True to skip json.loads. Below Python 3.11 that validation is a regular-expression scan of the payload and the path is 1.5x slower than the generic one (py3.9 1258 vs 839 us per 256 KiB frame, ABAB best of five); on 3.11 and 3.12 it is 0.55-0.58x. The package supports 3.9+, so the fast path is now gated on 3.11.
Listing the four workflow files in images.yml's paths made such a push start a run, but detect still only looked at boot/, silkd/, protocol/ and os-image/, so all three flags came back false, every job was skipped and the run went green having built nothing — worse than not triggering, because it looks like it built. Each area now carries its own build workflow in the paths it checks: build-boot.yml with boot/, build-silkd.yml with silkd/ and protocol/, build-os-images.yml and images.yml with os-image/.
images.yml now starts a run when its own build workflows change, and a changed carrier already refolds into every image. A change to build-os-images.yml or images.yml alone reached the downstream job but its matrix comes from a git diff of os-image/ only, so the matrix was empty, both matrix jobs were skipped by their include[0] guards and the run went green having built nothing. The detect step now treats a change to either of those two workflows as an all-image rebuild, the way a carrier change already is. Cache is untouched: no image input changed, so the rebuild is cached and proves the workflow, not the layers.
Merged
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.
The non-code and Python half of the LOC-justify round: four judgment lenses over the images and packaging, the workflows and Makefile and scripts, the docs against the code, and the Python SDK; every finding re-read in source before it landed. Nine commits, one purpose each. Nothing here touches sandboxd, silkd or the Go SDK.
Bugs
pty_openruns$SHELLand falls back to/bin/bash; the android userspace has neither the variable nor bash, andsilkd.rcset onlySILKD_PORTand the proxy trio. The init service now setsSHELL=/system/bin/sh. The android smoke only runs exec, which is why nothing caught it; a hardware check of the android pty needs the android lane, which this round's kit does not have — not run.ValueErroroutside theSandboxErrorhierarchy (Conn.recvcaughtOSErroronly); it is aProtocolErrornow.timeoutbounded each read of the dial, not the dial. docs/sdk-python.md promises it bounds the dial and upgrade; a node answering one header line per interval stretched a 0.2 s dial indefinitely — the "socket timeout is not a deadline" class from the last round, surviving in one place. The dial now spends every step from one deadline. A single header line trickled byte by byte is still bounded per read; that takes a hostile node.scripts/bench.shran underpipefailwith one unguarded pipeline on its results step; a host without cocoon lost the whole bench there.make basepassed noINPUTS_HASH, so a local edit to the secret-mountedinstall-agent.shsilently rebuilt against the cached layer; the target now computes the same content hash CI does.Reproducibility and CI
cargo clippy/cargo testwithout--lockedwhile release.yml and both Dockerfiles have it; the PyPI gate installed pytest and ruff unpinned where python.yml pins them.os-image/desktop/forced an uncached rebuild of the heaviest image (the depth-3 cache-bust rule and the inputs hash both counted markdown); markdown stays out of both now.imagestarget's help line said "all VM images" for two of nine.Python SDK hot path
decode_response's slice-and-b64decode(validate=True)fast path is 1.5× slower thanjson.loadsbelow Python 3.11, where validation is a regex scan (py3.9 1258 vs 839 µs per 256 KiB frame, ABAB best of five), and 0.55–0.58× on 3.11/3.12. The package supports 3.9+; the fast path is gated on 3.11.Docs
The README boot table promised
sandbox.debug/sandbox.tracebeside the Firecracker lane, but the pinned kernel builds no 8250 UART and cocoon boots that lane withconsole=ttyS0, so nothing they write is visible there; said now. The three error tables did not name the exec/ptyuserfield (an unresolvable user is abad_request; the guest distinguishes an unknown name from a failing name service).SandboxTimeoutwas missing from the Python errors list;ringsmokefrom the e2e roster.Owner calls, recorded in the round artifact rather than changed
FC lane console (+2 kernel config lines vs a boot-time cost); node-rt/python-rt built in parallel from byte-identical RUN blocks while the README says "same rootfs squashed"; flavors
FROM base:24.04(mutable; a branch dispatch can race a main run);ANDROID_BASE_IMAGEunpinned;aslhas no CI step; mypy runs on sdk/python only; shellcheck and the 55uses:lines are tag-pinned, not SHA; the Rust gate runs twice per push (silkd.yml ≡ build-silkd.yml); Python sessionexecputs the merged log in.stdoutwhere Go puts it inStderr; the 8 KiBmakefilebuffer on the relay (measure on Linux TCP first);cocoon.timeout/sandbox.initcmdline keys have no producer in either repo.Evidence
Python, in a venv mirroring python.yml (3.12, ruff 0.15.20, pytest 8.4.2, mypy 1.19.1):
ruff format --checkandruff checkclean; sdk/python 215 passed (213 + the two new tests, the dial test shown failing on the previous code), mypy clean, openai 10 and langchain 14 passed.make sh-lintexit 0, all eleven workflow files parse,bash -non the touched script,make helpandmake -n baserender the new argument. No Go or Rust source changed. Main's own artifacts (e8de664) passed the seven-leg E2E, the ring leg and the two-node cluster phases on cocoon-test1/test2 earlier today; this branch adds no runtime change on those lanes beyond the android init file.