diff --git a/.github/workflows/silkd.yml b/.github/workflows/silkd.yml index 30eb602e9..0fcd070c3 100644 --- a/.github/workflows/silkd.yml +++ b/.github/workflows/silkd.yml @@ -33,3 +33,7 @@ jobs: cargo fmt --check cargo clippy --locked --all-targets -- -D warnings cargo test --locked + + - name: root-only tests + working-directory: silkd + run: sudo -E env "PATH=$PATH" "HOME=$HOME" cargo test --locked --test git_e2e --test fs_e2e --test find_e2e -- --ignored diff --git a/docs/benchmarks.md b/docs/benchmarks.md index e733dace8..66dec788b 100644 --- a/docs/benchmarks.md +++ b/docs/benchmarks.md @@ -75,8 +75,9 @@ Knobs (environment variables): `WARM`/`WARM_N` (warm-pool depth and burst size — the burst must stay within the depth, or refill loses the race and the tail measures clones), `CLONE_N`, `COLD_N`, `RPC_N`, `PULL_MB`, `PULL_N`, `BURST_N` (concurrent clone-claim burst; 0 skips the stage), -`ADDR`/`TOKEN` (the throwaway daemon's listen address and token; change them -to run beside a live sandboxd). +`ADDR`/`TOKEN` (the throwaway daemon's listen address and token). Run the +harness on a node with no live sandboxd: the daemon reclaims every `sbx-` VM at +start, and the harness removes every `sbx-` VM at exit. Boot anatomy (where inside the cold tier the milliseconds go — kernel, initramfs phases, rootfs handoff) has its own harness: diff --git a/docs/browser.md b/docs/browser.md index 6927e8ac7..e03ef8efd 100644 --- a/docs/browser.md +++ b/docs/browser.md @@ -62,8 +62,9 @@ branch from. - Everything from the Ubuntu flavors (exec, files, sessions, git, pty) plus the running Chromium. -- **Preview URLs are not raw CDP**: the preview proxy rewrites `Host` to - `sandbox-id:port`, which Chrome's DevTools allowlist rejects. Use +- **Preview URLs are not raw CDP**: the preview proxy forwards the browser's + `Host`, which Chrome's DevTools allowlist rejects unless it is localhost or an + IP. Use `ProxyPort`/`DialPort` for CDP; preview URLs serve human-facing HTTP the workload chooses to expose (a live-view page, a screenshot server). - Guest env knobs read by `/usr/local/bin/chromium-cdp`: `CDP_PORT` (default diff --git a/docs/cluster.md b/docs/cluster.md index a8e6248fd..0a1b587f9 100644 --- a/docs/cluster.md +++ b/docs/cluster.md @@ -5,7 +5,7 @@ A cluster is a set of sandboxd nodes joined through a Gossip carries only placement hints — per-pool warm counts, promoted-template hashes, available volume names, each node's data-plane address, and a digest of the cluster-invariant config. -Per-sandbox state never leaves its owning node, so a stale view costs at most +Gossip carries no per-sandbox state, so a stale view costs at most one extra redirect, never correctness. A single node with no seeds is a valid mesh of one. @@ -63,7 +63,9 @@ probes, and preview forwarding retain the internal `advertise_addr`. See [TLS termination](deploy.md#tls-for-sdk-clients) for the edge setup. Node death is honest: a dead node's sandboxes die with it (memory state is -node-local by design). SWIM detects the death and peers stop redirecting to +node-local by design), except +[archived](#archived-claims-across-node-loss) and +[replicated](#running-claims-across-node-loss) claims. SWIM detects the death and peers stop redirecting to it. ### Volumes and placement @@ -564,8 +566,8 @@ one PostgreSQL table that every node reads, so one tenant per end user works: listener's next retry. Keepalive finds a dead connection in about 25 s and retries back off to 30 s, so the change lands within about a minute of the database returning; - - renew, fork and wake ask the database whenever the cache lacks the - tenant, so they never outlive a removal. While the database is down, they + - renew, fork and wake from the archive ask the database whenever the cache + lacks the tenant, so they never outlive a removal. While the database is down, they answer 503 for a tenant the node has not cached. ### Cluster-invariant config @@ -630,10 +632,10 @@ with every claim. - the same engine root path (cocoon's `root_dir`) on every node; it is not in the digest, so a mismatch shows only at the first cross-node branch - same `api_token`, tenant set, `preview_secret`, and `egress_ca` root everywhere + (a mismatch warns and shows in `sandboxd_config_digest_mismatch`) - with `meta_store`, every node's DSN points at the same database: the tenant set is not in the gossiped digest, so nodes on different databases serve diverging tenants without a warning - (a mismatch warns and shows in `sandboxd_config_digest_mismatch`) - `cluster_key` set if the gossip network is not otherwise trusted - one mesh sized in the hundreds of nodes, and promoted templates per pool or tenant rather than per end user (see [scale](#scale)) diff --git a/docs/deploy.md b/docs/deploy.md index ae585a846..68c1697c3 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -102,8 +102,8 @@ sandboxd reads one JSON file (`-config`, default As written the `egress` pool's guests reach nothing: on a bridge lane the NIC is locked default-deny and the proxy door is bound only for a claim that has a policy. Give the pool an [`egress`](egress.md) block before expecting traffic; -a tenant's claim of it (`acme` above) needs the tenant's own block as well and -gets the intersection of the two. +a tenant's claim of it needs an `egress_class` on the tenant as well and gets +the intersection of the two. | field | default | meaning | |---|---|---| @@ -119,7 +119,7 @@ gets the intersection of the two. | `no_rx_splice` | false | copy bytes bound for a guest's unix socket (egress downloads, silkd relay uploads such as file writes) through a pooled 256 KiB user-space buffer instead of kernel `splice`. Set it on GCE nodes whose gVNIC runs the DQO-QPL queue format (`dmesg` prints `Driver is running with DQO QPL queue format`, e.g. n4): there the kernel refuses to splice received pages into a guest's unix socket (`skb_splice_from_iter` returns `EIO`) and the transfer is cut short. Costs about 0.2–0.45 CPU cores per GiB/s into guests, and every open egress tunnel and silkd relay holds its 256 KiB buffer until it closes; throughput and latency are unchanged. Restart only | | `advertise_addr` | = `listen` | internal HTTP host:port for peer traffic and preview forwarding; also used by clients when `client_advertise` is unset. Must be routable when `listen` is a wildcard; a node with `mesh` set refuses to load while it names an unspecified host | | `client_advertise` | unset | client-facing HTTP(S) origin for this node, e.g. `https://node-a.sandbox.example.com`; published in owner, redirect, and peer responses. No path, query, fragment, or userinfo | -| `bridges` / `networks` | unset | egress-lane attachment: a list of host bridge devices, or a list of CNI conflist names. Mutually exclusive; with neither set the node serves only the no-network lane. A Linux bridge holds at most 1024 ports (kernel `BR_MAX_PORTS`), so an N-entry list raises the node's egress ceiling to N×1024 — VMs spread over the list by a stable hash of the VM name, so size it with headroom (the spread is statistical, not exact). `bridges` keeps the raw TAP-on-bridge attachment (taps in the root netns, no per-VM network namespace or CNI plugin execution); `networks` runs the CNI chain per VM. [Guarded egress](egress.md) on the egress lane (an egress-lane pool policy or any tenant policy) needs `bridges` and rejects a CNI network at load; none-lane pool policies ride the proxy on either | +| `bridges` / `networks` | unset | egress-lane attachment: a list of host bridge devices, or a list of CNI conflist names. Mutually exclusive; with neither set the node serves only the no-network lane. A Linux bridge holds at most 1024 ports (kernel `BR_MAX_PORTS`), so an N-entry list raises the node's egress ceiling to N×1024 — VMs spread over the list by a stable hash of the VM name, so size it with headroom (the spread is statistical, not exact). `bridges` keeps the raw TAP-on-bridge attachment (taps in the root netns, no per-VM network namespace or CNI plugin execution); `networks` runs the CNI chain per VM. [Guarded egress](egress.md) on the egress lane (an egress-lane pool policy or any `egress_classes` entry) needs `bridges` and rejects a CNI network at load; none-lane pool policies ride the proxy on either | | `volumes` | unset | node-local catalog of operator-managed dataset images: `[ {"name":"imagenet","path":"/srv/datasets/imagenet.img","directio":"off","tenants":["acme"]}, {"name":"scratch-db","path":"/srv/datasets/scratch.img","writable":true} ]`. Names match `^[a-z][a-z0-9_-]{0,19}$` and cannot start with `cocoon-`; paths are absolute; `directio` is `on`, `off`, or `auto` and defaults to `off` for both read-only and writable entries. `tenants` is an optional [access list](sandboxd-api.md#volume-access-lists); a listed name the tenant set does not hold grants nothing and logs a warning at boot (the [tenant API](sandboxd-api.md#tenants-v1tenants) may add it later). `writable` (default `false`) lets a claim request `mode: "rw"` on that entry — see [Dataset volumes](#dataset-volumes). The catalog is intentionally not part of the cluster digest | | `volume_roots` | unset | absolute directories that bound [runtime registration](sandboxd-api.md#put-v1volumesname): the registered path, once cleaned, must equal a root or sit under one, or the PUT returns 400. The check is lexical: `..` is resolved but symlinks are not followed, so a symlink under a root can point outside it. Static `volumes` entries are not checked. A saved registration outside the roots prevents startup. Unregister it before narrowing the roots, or widen the roots to include it. Keep `data_dir` outside every root. Unset or an empty list accepts any absolute path | | `secrets` | unset | credentials the egress proxy injects by name: `[{"name": "gh", "header": "Authorization", "value_env": "GH_TOKEN"}]`. A pool or egress-class rule references the name (on a claim where both apply, only the pool rule's secret is injected); the value comes from the claim's `guest: false` env, then the node's environment, never this file. Without `value_env` the secret's name is the env to read, and only claims supply it. See [egress](egress.md#claim-env-and-secrets) | @@ -141,7 +141,7 @@ gets the intersection of the two. | `checkpoint_ttl_hours` | 0 (keep forever) | ages out checkpoints older than this; the sweep runs hourly and at startup. Explicit deletes never wait for it. Must be nonzero and match fleet-wide when `checkpoint_peer_heal` is on — it is the expiry eligibility point for a healed replica a delete broadcast missed, after which its next successful hourly sweep removes it; persistent sweep failure extends retention until one succeeds, so it is not a hard ceiling | | `checkpoint_peer_heal` | false | on a cluster, lets a node pull a checkpoint it lacks from a peer — found via a live probe, not gossip — rather than failing the branch; see [placement lifecycle](cluster.md#checkpoints-on-a-cluster). Three requirements, all enforced at config load: a nonempty `api_token` (the blob transfer between peers authenticates with it; without one the raw record stream would be open), `mesh.cluster_key` set (the pull presents the fleet `api_token` to an address learned from the peer probe, so the gossip layer carrying that address must itself be authenticated), and `checkpoint_ttl_hours` nonzero (a replica a delete broadcast missed becomes eligible for expiry after it, and its next successful hourly sweep removes it — so it is the finite eligibility point, not an exact ceiling). A shared checkpoint store (`checkpoint_store` kind `s3`) ignores this setting — every node already resolves every checkpoint directly, so there is nothing to heal | | `warm_max` (pool entry) | 0 (static) | turns on the demand-adaptive watermark for that pool: the warm target rises from `warm` toward `warm_max` while claims arrive faster than the measured provision lead covers, and decays back over ~a minute of silence. The pool follows the target down: warm VMs above it are destroyed once the count has stayed above it for a full decay period, so a burst does not leave the pool parked at its high-water mark | -| `warmup` (pool entry) | unset | argv run in the golden VM after readiness and before its snapshot, so the files it touches are page-cache-resident in every clone, and again in every clone before it joins the warm pool, so those pages are already faulted into the restored VM when the first command runs — e.g. `["node", "-e", "0"]` on a Node flavor. It runs under the engine's 2-minute command timeout in silkd's base environment (`PATH`, `TERM`, and the guest image's proxy variables wherever nothing routes directly — the none lane and the locked bridge egress lane — with the proxy not yet serving, since a door pre-bound at refill only starts serving at claim); a non-zero exit or a timeout fails the golden build, so the pool stays unfilled until the config is fixed. Config-owned like `egress`: `PUT /v1/pools` rejects it, and a golden built with a different warmup is rebuilt | +| `warmup` (pool entry) | unset | argv run in the golden VM after readiness and before its snapshot, so the files it touches are page-cache-resident in every clone, and again in every clone before it joins the warm pool, so those pages are already faulted into the restored VM when the first command runs — e.g. `["node", "-e", "0"]` on a Node flavor. It runs under the engine's 2-minute command timeout in silkd's base environment (`PATH`, `TERM`, and the guest image's proxy variables wherever nothing routes directly — the none lane and the locked bridge egress lane — with the proxy not yet serving, since a door pre-bound at refill only starts serving at claim); a non-zero exit or a timeout fails the golden build, so the pool stays unfilled until the config is fixed. Config-owned like `egress`: `PUT /v1/pools` rejects it, and a golden built with a different warmup is rebuilt The build then waits up to 30 s for the guest's systemd to finish starting, because a snapshot taken during boot makes every clone run the queued jobs before silkd answers. | | `capture_trim` (pool entry) | `false` | Trim the guest's copy-on-write disk before a promote or checkpoint of this pool's sandboxes (and of the clones of a template promoted from it), so the record carries only live data: blocks a build deleted — a `COPY` step's archive, a `RUN` step's caches — are no longer captured. sandboxd mounts the disk init resolved from `cocoon.cow` a second time at `/run/sandboxd-trim` through silkd as root, runs `fstrim` and unmounts, under the capture's transition lock (about 15 ms on a warm clone). A failed trim is logged and the capture proceeds, larger. A hibernated or archived sandbox is captured untrimmed. Config-owned like `egress`: `PUT /v1/pools` rejects it | | `reader` (pool entry) | `false` | Serve read-only file calls on this pool's hibernated or archived claims from a warm VM of the same pool instead of waking them; needs `warm` of at least 1 and a cocoon whose `vm disk attach` takes `--from-snapshot` and `--from-dir`. See [Reading sleeping claims](#reading-sleeping-claims-without-waking-them). Config-owned like `capture_trim`: `PUT /v1/pools` rejects it | | `egress_classes` | unset | named tenant egress layers: `[{"name": "desk", "egress": {…}, "egress_upstream_env": "DESK_UPSTREAM"}]`. A tenant names one with `egress_class` through the [tenant API](sandboxd-api.md#tenants-v1tenants) and its claims take that policy intersected with the pool's. `egress` is required; `intercept` stays a pool-rule flag. Reloadable: a class's policy reaches live claims on their next request. A class removed while tenants still name it leaves them with no egress and is logged at load and at reload | @@ -154,7 +154,7 @@ gets the intersection of the two. | `idle_hibernate_seconds` | 0 (off) | node-wide idle policy for unpooled claims (template/checkpoint claims): a none-lane claim is hibernated once it has had no open data-plane connection (relay, buffered exec, preview) and no egress request in flight for this long; the clock restarts when the last connection closes, so a long command is never cut short. The next call that reaches the guest wakes it transparently. Per-pool `idle_hibernate_seconds` does the same for that pool's claims; pooled keys ignore the node-wide value, and egress pools reject it because they cannot resume safely. Opt in deliberately: a wake costs latency and the snapshot, so callers with their own idle logic must not pay twice | | `archive_after_seconds` | 0 (off) | tier below hibernation: a hibernated claim idle this long is checkpointed to the store and its local VM dropped, freeing the node entirely; the next call that reaches the guest restores it transparently (a checkpoint restore's latency) with a fresh lease of the length the claim was granted (the server default of 5m when it asked for none); a lease moves only when a client calls [`/renew`](sandboxd-api.md#post-v1sandboxesidrenew). Keep `idle_hibernate_seconds` under the leases your claims use, or a woken sandbox that idles again is destroyed by the reap before it can hibernate, its archive already consumed by the wake; one still in use when its lease ends is destroyed the same way, unless it claimed `on_expire: archive`, which archives at the lease end whatever this setting. Requires `idle_hibernate_seconds > 0` and must exceed it. Node-wide for unpooled keys; per-pool overrides for that pool | | `archive_delete_after_seconds` | 0 (keep) | purge an archived claim's store checkpoint this long after it was archived; the claim is then gone for good. On S3, chunks and manifests remain until [offline reclamation](#reclaiming-s3-storage). On archive this retention window replaces the live claim deadline; 0 clears the deadline so the archive is kept forever. Same node-wide/per-pool split | -| `archive_failover` | false | let another node adopt this node's archived claims once its node lease expires, and the running claims of pools with `replicate_seconds`: each archive also writes a row (token SHA-256, owner, epoch, checkpoint id, retention deadline) to the shared PostgreSQL, and a `Lookup` that reaches any node takes the claim over there (see [archived claims across node loss](cluster.md#archived-claims-across-node-loss)). Requires `meta_store` with a `cell` (rows are scoped by it, so meshes sharing one database never adopt from each other), a `checkpoint_store` of kind `s3` and a `mesh`, all checked at config load. The node also renews a lease row every 5 s (see [node lease](cluster.md#node-lease)). With the meta store down, archives, wakes and releases of archived claims fail and retry, and replicated claims stop being served once the lease is overdue (see [running claims](cluster.md#running-claims-across-node-loss)); other running claims never touch it. Restart only | +| `archive_failover` | false | let another node adopt this node's archived claims once its node lease expires, and the running claims of pools with `replicate_seconds`: each archive and replication round also writes a row (token SHA-256, owner, epoch, checkpoint id, retention deadline, and the claim record with its env, `guest: false` values included, in plaintext) to the shared PostgreSQL, and a `Lookup` that reaches any node takes the claim over there (see [archived claims across node loss](cluster.md#archived-claims-across-node-loss)). Requires `meta_store` with a `cell` (rows are scoped by it, so meshes sharing one database never adopt from each other), a `checkpoint_store` of kind `s3` and a `mesh`, all checked at config load. The node also renews a lease row every 5 s (see [node lease](cluster.md#node-lease)). With the meta store down, archives, wakes and releases of archived claims fail and retry, and replicated claims stop being served once the lease is overdue (see [running claims](cluster.md#running-claims-across-node-loss)); other running claims never touch it. Restart only | | `mesh` | unset | join a cluster ([Clusters](cluster.md)); unset = single node | | `pools[]` | — | warm pools, keyed by `(template, net, size)`. `warm` defaults to 4 only when both `warm` and `warm_max` are omitted; an explicit `warm: 0` is kept, so `warm: 0` under a `warm_max` starts the pool empty and grows it on demand, and `warm: 0` alone registers the key with no warm VMs (`PUT /v1/pools` never defaults `warm`); `net` is `none` or `egress`; `size` is a tier, below. Retune online without a restart via [`PUT /v1/pools`](sandboxd-api.md#put-v1pools) — omitted pools drain, and their goldens are deleted once they are idle (`warm: 0` keeps the golden). This is the **first-boot seed**: once a node takes a `PUT /v1/pools`, the applied set persists to `/pools.json` and overrides this section on every later boot (a startup log notes it); delete `pools.json` to return to config-owned pools. With a `meta_store` `cell`, the cell's set overrides it instead (see [shared pool set](#shared-pool-set)). Egress stays config-owned either way. A template re-pulled or re-imported under the same ref is picked up within a minute: the pool rebuilds its golden and replaces its warm VMs. See [state ownership](cluster.md#state-ownership) | @@ -189,7 +189,7 @@ the claim as before ([API](sandboxd-api.md#get-v1sandboxesidagent)). It needs Cloud Hypervisor and a cocoon whose `vm disk attach` takes `--from-snapshot` and `--from-dir` (v0.7.3 or newer). Leave `reader` off -on a Firecracker node or with an older cocoon: there every read spends a warm VM +with an older cocoon: there every read spends a warm VM on an attach that fails, then wakes the claim. An archive exported by an older cocoon, whose `snapshot.json` has no `disk_attach_files`, wakes before any VM is taken. Unpooled claims (template and checkpoint claims) always wake. @@ -237,10 +237,11 @@ gets a fresh one. The claim's own code runs in its reader with root, as it did in the claim, and the reader serves one claim and is destroyed afterwards, so the claim's bytes never reach a shared VM or the host kernel. -Whenever a reader cannot be built (no warm VM in the claim's pool, the disk -reserve is low, a failed attach, an archive without +Whenever a reader cannot be built (no warm VM in the claim's pool, a failed +attach, an archive without `disk_attach_files`, an image without `silkd` at that path), the call wakes the -claim as it does without the feature. With the feature on for the pool, every +claim as it does without the feature. While the disk reserve is low the wake is +refused too, so the read fails with an `error` frame. With the feature on for the pool, every hibernate of its claims runs a guest `sync` right before the pause, and a reader shows the disk as of that `sync`: a write the guest makes in the instant between the `sync` and the pause is visible only after a wake. A hibernate whose `sync` @@ -1085,3 +1086,8 @@ sandboxd: one `preview_advertise` share one browser origin, so different claims' apps are same-origin to the browser; workloads needing browser-side isolation need per-sandbox subdomains on the fronting proxy. +- **Credentials**: the proxy drops `Cookie` and `Authorization` from each + request, so a guest app that needs them cannot authenticate a browser through + a preview URL. +- **Health**: `GET /healthz` on `preview_listen` answers 200 `ok`, for the load + balancer's check. diff --git a/docs/e2b.md b/docs/e2b.md index 9c8e5a7dd..01274fc1f 100644 --- a/docs/e2b.md +++ b/docs/e2b.md @@ -86,7 +86,7 @@ sources that cannot be pinned (a `curl | bash` setup script, an unversioned git dependency, moving release archives), so they wait for a reproducible source. Every pip dependency is pinned by `constraints.txt` and `server-constraints.txt` -beside the Dockerfile, and bytecode is compiled hash-checked at the end, so a +beside the Dockerfile, and bytecode is compiled hash-based (unchecked) at the end, so a rebuild of one commit against the same base yields the same layer. That holds for the OCI and registry exporters with `SOURCE_DATE_EPOCH` and `rewrite-timestamp=true`, as the image workflow builds; the `docker` exporter diff --git a/docs/egress.md b/docs/egress.md index 89aadb65c..0844eb56a 100644 --- a/docs/egress.md +++ b/docs/egress.md @@ -84,7 +84,7 @@ lane needs. The lock is fail-closed — a claim whose NIC cannot be locked is rejected, not handed out unlocked, and no policy still means a locked NIC (default-deny), never a free one. It lives in the host root netns and is removed once the VM is gone (a failed -remove keeps an existing lock in place; the next restart retries the remove). A +remove keeps an existing lock in place; each reap tick retries the remove). A lock that never applied plus a failed remove leaves the VM unguarded until a later remove succeeds. @@ -248,7 +248,9 @@ woken sandbox binds at arm time. it only for a claim with a [credential](#claim-credentials) for that host; for every other claim the rule is a plain one, so its tunnels splice and HTTP/2, WebSocket and pinned clients keep working. An `"inject"` rule cannot carry a - `secret`. Only a pool rule may set `intercept`. + `secret`. Only a pool rule may set `intercept`. A `true` rule does not serve + the forward door: a plain `http://` or absolute-form `https://` request that + only it matches answers 403. - No policy on a claim ⇒ no egress at all (the proxy is not started). Each decision is metered as an `egress` usage event, which names the injected @@ -278,8 +280,11 @@ gateway key per user, while the guest holds a placeholder. which hosts receive a secret, and the claim only supplies its value. A `guest: true` entry never feeds a secret. - **Precedence.** The claim's entry wins over the node's value for that claim; - an entry set to `""` suppresses the node's value. With neither, the matched - request passes as the guest sent it. + an entry set to `""` suppresses the node's value. A secret with `value_env` + needs that variable set on the node: unset or empty, it stops the daemon at + start and a reload answers 400. A secret only claims supply omits `value_env`; + on a claim with no entry for it, the matched request passes as the guest sent + it. - **Lifetime.** The env lives in the claim record in `claims.json` (0600 under `data_dir`), survives a restart, hibernate, wake, archive and unarchive, and goes with the release or reap. A fork child, a checkpoint @@ -287,7 +292,9 @@ gateway key per user, while the guest holds a placeholder. sets. Processes captured live in a snapshot (sessions, detached execs, units) keep the source's guest values in their own environment; a `guest: false` value never enters the guest, so no snapshot, checkpoint or - store object holds one. + store object holds one. With [`archive_failover`](deploy.md#configuration), + the row of an archived or replicated claim carries the env, `guest: false` + values included, in plaintext in the shared PostgreSQL. - **Audit.** An injection is recorded as the secret's name; the value is never logged, listed or returned. @@ -355,6 +362,9 @@ guest's next connection; a tunnel the guest keeps open stays as it was. The placeholder is not a secret: it stays in the guest and in `GET /env`. The entry rides the claim request or any `/env` write, and rotation is the same `PATCH`, accepted on a hibernated or archived claim without waking it. +The write does not reach an archived claim's `archive_failover` row: after that +claim's node is lost, an adopter starts from the row, which holds the value +from the archive. - **Envelope.** The operator still decides which hosts can carry a credential: every host must be covered by an `intercept` rule (`true` or @@ -431,7 +441,7 @@ that fails answers the guest 502 (SOCKS5: host unreachable) and never falls back to a direct dial. An allowed connection's audit and usage records name the upstream's `host:port`, never its credentials, and `GET env` blanks the value like every `guest: false` entry. Cost: a node without `egress_upstream` pays nothing; with -it, each allowed request reads the claim's env under the manager lock. +it, each allowed request reads the claim's env snapshot, without a lock. `egress_usage_bytes: true` adds one `egress_bytes` usage event per allowed tunnel or request when it ends, with the payload bytes it moved (`tx` guest to diff --git a/docs/mcp.md b/docs/mcp.md index cd3cd5899..d3c09a1ae 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -44,6 +44,8 @@ in the guest, which can hold its reply up to 5 s past the cap. | `release` | destroy the sandbox and its files; the session forgets the id, so a second release is rejected as unknown | | `node_info` | warm pools, promoted templates, live claims, drain state, capacity and mesh peers; needs the operator token | +An egress-lane sandbox cannot `fork`, `checkpoint`, `hibernate` or `promote`. + Sandbox handles (and their tokens) are held by the server process for the session, and released with it: when the MCP client disconnects or the server is signalled, it destroys every sandbox that session claimed, whatever diff --git a/docs/sandboxd-api.md b/docs/sandboxd-api.md index 228da30e1..962e14ff0 100644 --- a/docs/sandboxd-api.md +++ b/docs/sandboxd-api.md @@ -68,7 +68,7 @@ Auth: `Authorization: Bearer ` (when configured). cold-booting an image named like the template - `env` is the claim's own environment, at most 64 entries: each name a shell identifier of at most 128 bytes, each value at most 8 KiB with no control - characters other than tab, and names plus values at most 64 KiB in total. An entry holds only `value`, `guest` and `inject`; any other + characters other than tab, and names, values and inject fields at most 64 KiB in total. An entry holds only `value`, `guest` and `inject`; any other member, or `"guest": null`, answers 400, so a mistyped flag never lands a host-only value in the guest. `guest` (default `true`) delivers the entry into the guest; a `guest: false` entry never enters it and only feeds the node's egress @@ -475,7 +475,9 @@ independent guest processes and sessions continue. See the ## POST /v1/sandboxes/{id}/release Auth: the sandbox's own token, or the root token for operator cleanup by id. -Destroys the VM. 204 on success, 404 for an unknown id or wrong token. +Destroys the VM. 204 on success, 404 for an unknown id or wrong token, 500 when +the VM survives removal: the claim is already gone and the node retries the +removal. Releasing an already-gone sandbox is 404 — the SDK treats it as success. ## POST /v1/sandboxes/{id}/hibernate @@ -507,7 +509,8 @@ while stored, and waking it starts a fresh lease of the length the claim was gra [failed](deploy.md#dead-vmms-and-host-restarts) sandbox is cold-booted from its own disk (memory is lost) and its restart budget resets. 204 on success, 404 unknown id or wrong token, 409 a failed egress-lane or volume -sandbox, or one whose VM record is gone, 500 when the cold boot itself fails, +sandbox, or one whose VM record is gone, 403 a removed tenant's archive, +429 while the disk reserve is low, 500 when the cold boot or a restore fails, 503 `lease lost; retry` for a replicated claim while its node's lease is overdue. ## POST /v1/sandboxes/{id}/renew @@ -582,7 +585,7 @@ the body: Publishes the sandbox's current state as a node-local template under (template, this sandbox's net, its size): later claims for that key clone -from it, provision-on-demand — no warm pool unless the node config adds one. +from it, provision-on-demand, with no warm pool. Re-promoting to the same name replaces the template. A hibernated sandbox is promoted from its memory image without waking. 200 returns the template's full key and immutable content identity. On the default local-disk backend a @@ -679,8 +682,8 @@ token; 409 egress pool on a node without an egress attachment; 503 the ## Tenants (/v1/tenants) -Auth: root only (tenant tokens get 403); a node without `api_token` answers 400, -since tenants need it. The API is the only way in: `config.json` has no +Auth: root only (tenant tokens get 403); a node without `api_token` answers 400 +to a write, since tenants need it. The API is the only way in: `config.json` has no `tenants` field, and a node starts with no tenants. The set changes at runtime, with no restart: @@ -893,7 +896,7 @@ an SDK caller should set. Auth: node API token. Root sees every live claim; a tenant sees only its own. The index is `{"sandboxes": [{id, key, tenant?, deadline, claimed_at?, hibernated, archived?, from_checkpoint?, claim_ref?, metadata?, on_expire?, cpu_count, mem_total_bytes, -volumes?: [{name, mount?, mode?, attach_only?}], volumes_want?: [{name, mount?, mode?, attach_only?}], restarts?, restarted_at?, failed?}]}` — `cpu_count` and `mem_total_bytes` are the +volumes?: [{name, mount?, mode?, attach_only?}], volumes_want?: [{name, mount?, mode?, attach_only?}], restarts?, restarted_at?, failed?}]}` — `deadline` is `0001-01-01T00:00:00Z` on an archive kept forever; `cpu_count` and `mem_total_bytes` are the size tier's allocation; `restarts` counts the cold boots that replaced an exited VMM and `restarted_at` stamps the last one (each loses guest memory and the instance-metadata document, so a caller re-establishes guest state when @@ -1113,8 +1116,9 @@ clock, so a client that keeps a connection warm must close it when idle for before the 101, reach silkd in order; the Go and Python SDKs send their first frames that way over plain HTTP, so a proxy in front of a plain-HTTP endpoint must forward them too (Go's `httputil.ReverseProxy` drops them; Caddy forwards -them). 426 without the upgrade header, 404 unknown sandbox, 409 a failed -sandbox, 502 guest unreachable, 503 `lease lost; retry` for a replicated claim +them). 426 without the upgrade header, 403 a removed tenant's archive, 404 +unknown sandbox, 409 a failed sandbox, 429 while the disk reserve is low, 500 a +failed restore, 502 guest unreachable, 503 `lease lost; retry` for a replicated claim while its node's lease is overdue (see [running claims](cluster.md#running-claims-across-node-loss)). A relay of a claim of a replicating pool that is already open when the lease turns overdue @@ -1175,7 +1179,8 @@ fork in flight, answers `409` at once; an open passive relay still holds the san 400 a port outside 1-65535; 401 missing bearer token; 404 unknown sandbox or wrong token; 409 a failed sandbox or a passive relay to a paused one; 426 without the upgrade header; 502 when the guest cannot be reached on that port — no listener, or a -hibernated sandbox whose restore failed. The node log carries which; 503 +hibernated sandbox whose restore failed. The node log carries which; 403 a +removed tenant's archive; 429 while the disk reserve is low; 503 `lease lost; retry` for a replicated claim while its node's lease is overdue. ## POST /v1/sandboxes/{id}/exec @@ -1201,7 +1206,8 @@ does not exist are the caller's, not 502s; 401 missing bearer token; 404 unknown 413 when stdout+stderr exceed 8 MiB; 503 with `Retry-After: 1` when the node's buffered-exec memory is exhausted (64 MiB of output across every buffered exec in flight; the command is killed); 502 guest unreachable or any other -silkd error; 409 a failed sandbox; 503 `lease lost; retry` for a replicated claim +silkd error; 403 a removed tenant's archive; 409 a failed sandbox; 429 while the +disk reserve is low; 500 a failed restore; 503 `lease lost; retry` for a replicated claim while its node's lease is overdue; 504 when the command outlives `timeout_seconds`. A client that hangs up first gets no reply and the command is killed. A hibernated sandbox wakes transparently like on the relay. ## GET /v1/sandboxes/{id}/owner diff --git a/docs/sdk-python.md b/docs/sdk-python.md index 87d404df8..3373e3c75 100644 --- a/docs/sdk-python.md +++ b/docs/sdk-python.md @@ -14,7 +14,7 @@ synchronous by design (agent frameworks that need async wrap calls in does). It matches the [Go SDK](sdk.md) guest and data-plane surface, with these exceptions: the operator pool and tenant verbs (`SetPools`, `PutTenant` and their siblings), the claim env (`WithEnv`, `Env`/`SetEnv`/`PatchEnv`) and -setting claim `metadata` or `on_expire` remain Go-only, `Template.new` takes no +setting claim `metadata` or `on_expire`, and reading a claim's `net_route` remain Go-only, `Template.new` takes no `claim_ref`, and file and tar payloads are whole `bytes` values rather than streams (`read_file`/`pull` return them, `write_file`/`push` take them; the wire is chunked, the caller's copy is not). Wire fidelity is @@ -23,8 +23,8 @@ Python all round-trip in CI. ## A complete example -Claim a sandbox, push a project in, run a build, then freeze the built state -and fan out two independent workers from that exact moment: +Claim a `node:24.04` sandbox (it carries `make`; `rt:24.04` does not), push a +project in, run a build, then freeze the built state and fan out two independent workers from that exact moment: ```python import io @@ -46,7 +46,7 @@ def project_tar() -> bytes: def main() -> None: client = Client(os.environ["SANDBOXD_ADDR"], api_token=os.environ["SANDBOXD_TOKEN"]) - with client.new("rt:24.04", size="medium", ttl_seconds=600) as sb: + with client.new("node:24.04", size="medium", ttl_seconds=600) as sb: print(f"claimed {sb.id} on {sb.owner}") sb.push("/work", project_tar()) # only ingestion path on the no-network lane @@ -123,7 +123,8 @@ branched). `sb.template_digest` is the exact content identity when the claim cloned a promoted template; it is empty for other sources. `Sandbox` is a context manager; `sb.close()` releases it (releasing one already gone is not an error — double-release and reap races stay silent). -Volume sandboxes cannot hibernate, fork, checkpoint, or promote. Checkpoint +Volume sandboxes cannot hibernate, fork, checkpoint, or promote. Egress-lane +sandboxes cannot either; each answers 409. Checkpoint branches do not accept volumes of either mode in this version. `mount=False` on `client.new` or `template.new` claims the same volumes @@ -389,7 +390,7 @@ code = sb.run(["bash", "-c", "make test"], ``` `timeout` on `exec` and `run` is a wall clock over the dial and the run: at -its end the connection is cut, which makes silkd kill the command, and +its end the SDK cuts the connection and kills the reported pid, and `TimeoutError` is raised. `exec` returns stdout and raises `ExitError` on a non-zero exit — carrying diff --git a/docs/sdk.md b/docs/sdk.md index 02d22dcca..311d84a44 100644 --- a/docs/sdk.md +++ b/docs/sdk.md @@ -11,7 +11,8 @@ cluster. ## A complete example -Claim a sandbox, push a project into it, run a build, then freeze the built +Claim a `node:24.04` sandbox (it carries `make`; `rt:24.04` does not), push a +project into it, run a build, then freeze the built state and fan out two independent workers from that exact moment: ```go @@ -40,7 +41,7 @@ func main() { log.Fatal(err) } - sb, err := client.New(ctx, "rt:24.04", + sb, err := client.New(ctx, "node:24.04", sandbox.WithSize(sandbox.Medium), sandbox.WithTimeout(10*time.Minute)) if err != nil { @@ -227,7 +228,8 @@ promoted template; it is empty for other sources. `Owner()` names the owning node, and `Token()` returns the per-sandbox bearer to persist with `ID` for a later `Lookup`; `Close()` releases the sandbox (releasing one already gone is not an error, and `Close` is bounded internally so it stays defer-friendly). -Volume sandboxes cannot hibernate, fork, checkpoint, or promote. Passing +Volume sandboxes cannot hibernate, fork, checkpoint, or promote. Egress-lane +sandboxes cannot either, nor take `WithArchiveOnExpire`; each answers 409. Passing `WithVolumes` or `WithClaimRef` to `Checkpoint.New` returns a local error: checkpoint branches support neither volumes nor a claim reference in this version. `WithMetadata`, `WithArchiveOnExpire` and `WithEnv` apply to `New`, `Template.New`, and @@ -438,7 +440,7 @@ answers 404 instead of taking a warm VM or cold-booting an image named like it. `Promote` publishes the sandbox's state as a template on its owning node, keyed by (name, the sandbox's network lane, its size). Claims clone on demand (~a golden-clone's latency); there is no warm pool for promoted -templates unless the node's config adds one. Re-promoting to the same name +templates. Re-promoting to the same name replaces the template. `Template.ContentDigest` identifies the published export bytes; a claim from that exact generation carries the same value in `Sandbox.TemplateDigest`. A caller pinning a mutable template name can compare @@ -742,7 +744,7 @@ sb = client.Attach(ownerAddr, id, token) // bind a known handle, no lookup roun err = client.PutTenant(ctx, sandbox.TenantSpec{Name: "u:42", Token: tok, MaxClaims: 10}) res, _ = client.PutTenantCluster(ctx, sandbox.TenantSpec{Name: "u:42", MaxClaims: 20}) // token kept res, _ = client.DeleteTenantCluster(ctx, "u:42") -tenants, _ := client.Tenants(ctx) // names, caps, live claims, removed tenants; never tokens +page, _ := client.Tenants(ctx, "", 0) // page.Tenants: names, caps, live claims, removed tenants; never tokens; page.Next pages ``` `Sandboxes` is scoped to the calling token, so a tenant sees only its own diff --git a/docs/security.md b/docs/security.md index 5e4bbcf49..588ee190d 100644 --- a/docs/security.md +++ b/docs/security.md @@ -40,7 +40,9 @@ exposing any part of a deployment beyond a single trusted host. proxy's answers, and every credentialed call is journaled. Git auth tokens travel as in-memory headers, never guest disk. Secrets come from the host environment or a claim's `guest: false` env, never the config - file; that env stays in the node-local claim record. + file. That env stays in the node-local claim record; with + `archive_failover`, the row of an archived or replicated claim also carries + it, `guest: false` values included, in plaintext in the shared PostgreSQL. - **Sandbox identity.** Every clone is reseeded (entropy, machine-id), so branches and forks never share an identity with their source. @@ -64,7 +66,9 @@ all of them. 4. **Treat the checkpoint store like backups.** A checkpoint, an archive and a replica each embed full guest memory, including any secret the workload held at capture. Restrict store access; use S3 server-side encryption or - an encrypted mount where that matters. + an encrypted mount where that matters. With `archive_failover`, treat the + shared PostgreSQL the same way: its rows hold claim env in plaintext, + `guest: false` values included. 5. **No custom NAT64 prefix routed on a sandboxd host** — the SSRF guard cannot see through an operator-specific translator prefix ([egress](egress.md#deployment-constraints)). diff --git a/docs/silkd.md b/docs/silkd.md index 68b4cc185..1ea79925c 100644 --- a/docs/silkd.md +++ b/docs/silkd.md @@ -49,7 +49,7 @@ suites — a frame only one side can parse fails CI. | exec | `exec {argv, cwd?, env?, user?, detach?, session?}` | `started{pid}` → `stdout/stderr{data}`… → `exit{code}`; the client may stream `stdin{data}` / `stdin_close`. `detach` returns after `started`; the process keeps a bounded output ring for later `logs`/`attach` and leaves the table 5 minutes after it exits, after which its pid answers `not_found`; not combinable with `session`. With `session` only `argv` is used — `cwd`, `env`, `user` and `stdin` frames are ignored (the session owns them, and its commands read `/dev/null`) — no `started` is sent, and stderr arrives merged into `stdout`. A child's environment is cleared to `PATH`, `TERM` and the lane's proxy variables, then the [claim env](#claim-env), then `env`; `HOME` and `USER` are set only with `user` | | procs | `ps` / `kill {pid, signal?}` / `attach {pid}` / `logs {pid}` | `ps` answers one `procs{procs}` frame, `kill` a bare `done`; `logs` replays `stdout/stderr{data}`…, adds `exit{code}` once the process has exited, and closes with `done`; `attach` replays and follows `stdout/stderr{data}`… to `exit{code}`, or to `done` when the process is gone before its exit. Handles are guest pids; any connection can list, signal, replay, or re-attach live | | sessions | `session_create {id?, cwd?, env?}` / `session_list` / `session_rm {id}` | `session_created{id}` / `sessions{sessions}` / `done`. A session is a real persistent bash; `exec` with `session` runs inside it. Idle sessions are reaped after 30 minutes | -| fs | `fs_write {path, mode?}` (+`data`/`data_end` frames) / `fs_read` / `fs_list` / `fs_stat` / `fs_mkdir {parents?}` / `fs_rm {recursive?}` / `fs_rename {from, to}` | `fs_read` streams `data{data}`… → `done` for a regular file and answers `bad_request` for a directory, device or FIFO, `fs_list` streams 4096-entry `entries{entries}` batches → `done`, `fs_stat` answers one `stat{info}`, and the mutating verbs terminate with `done`. Streaming runs both directions; write commits atomically via temp+rename and inherits an overwritten file's mode | +| fs | `fs_write {path, mode?}` (+`data`/`data_end` frames) / `fs_read` / `fs_list` / `fs_stat` / `fs_mkdir {parents?}` / `fs_rm {recursive?}` / `fs_rename {from, to}` | `fs_read` streams `data{data}`… → `done` for a regular file and answers `bad_request` for a directory, device or FIFO, `fs_list` streams 4096-entry `entries{entries}` batches → `done`, `fs_stat` answers one `stat{info}`, and the mutating verbs terminate with `done`. Streaming runs both directions; write commits atomically via temp+rename and inherits an overwritten file's mode and, under root, its owner | | tree | `fs_push {dest}` (+tar as `data` frames) / `fs_pull {path}` | whole trees as tar streams through the guest tar: `fs_pull` streams `data{data}`… → `done`, `fs_push` terminates with `done` | | search | `fs_find {path, pattern, glob?}` → `match{file, line, content}`… → `done` / `fs_replace {files, pattern, replacement}` → `replaced{file, replacements}`… → `done` | regex as data, no shell quoting; `glob` is anchored `*`/`?` wildcards over file names; find skips binary and >8 MiB files, and replace skips the same 8 MiB bound with a zero count. One match whose frame would pass the 8 MiB cap ends the find with an `error`. Replace is atomic per file, not per list: it stops with an `error` at the first missing, unreadable or non-UTF-8 file, and the files before it stay rewritten | | watch | `fs_watch {path, recursive?}` | `ready` once armed (events after it are guaranteed captured), then `event{kind, path}` until the client disconnects; watcher or delivery-queue overflow errors arrive as a terminal `error` instead of silently losing events | @@ -115,9 +115,10 @@ older silkd ignores the file; units that read it still see it. - 8 MiB frame cap (a malformed peer cannot OOM the daemon) - foreground exec output is backpressured while the child runs: a slow client - paces it rather than losing output. Once the child exits, the client has 2 - seconds to drain what is still queued (up to 8 MiB) before `exit` is sent - and the rest is dropped. Detached observers ride a best-effort broadcast + paces it rather than losing output. Once the child exits, the pump has 2 + seconds to queue the rest of its output, and what is not queued by then is + dropped. What is queued (up to 8 MiB) reaches the client ahead of `exit`, with + no time limit. Detached observers ride a best-effort broadcast - no fsync durability on writes (guest page cache semantics) - `fs_push` is network-failure atomic: the stream extracts into a staging dir inside `dest` and merges (local renames) only after the tar terminates diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index a97279b64..a3531c73d 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -1,7 +1,4 @@ -// Package e2e drives the full host stack in one process: real pool manager, -// real engine dialing real hybrid-vsock sockets, real HTTP server and relay, -// real SDK — only cocoon and the guest are faked. It doubles as the drift -// guard between the SDK's wire mirrors and sandboxd's types. +// Package e2e drives the full host stack in one process with only cocoon and the guest faked. package e2e import ( diff --git a/mcp/server.go b/mcp/server.go index a38912c9b..089db43f5 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -49,11 +49,10 @@ type server struct { client *sandbox.Client template string - mu sync.Mutex - boxes map[string]*sandbox.Sandbox - ckpts map[string]*sandbox.Checkpoint - release sync.Once - released chan struct{} + mu sync.Mutex + boxes map[string]*sandbox.Sandbox + ckpts map[string]*sandbox.Checkpoint + release sync.Once } func newServer(addr, token, template string) (*server, error) { @@ -70,7 +69,6 @@ func newServer(addr, token, template string) (*server, error) { template: template, boxes: map[string]*sandbox.Sandbox{}, ckpts: map[string]*sandbox.Checkpoint{}, - released: make(chan struct{}), }, nil } @@ -170,9 +168,7 @@ func (s *server) closeBoxes() { wg.Go(func() { _ = sb.Close() }) } wg.Wait() - close(s.released) }) - <-s.released } func (s *server) trackBox(sb *sandbox.Sandbox) { diff --git a/mcp/tools.go b/mcp/tools.go index 9861880c6..209682d7b 100644 --- a/mcp/tools.go +++ b/mcp/tools.go @@ -54,11 +54,11 @@ var ( schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint"), "path": str("absolute path of the directory")}, "sandbox_id", "path"), toolListDir, }, { - "fork", "Clone a sandbox into N independent children that start from its exact memory and disk state, including running processes; each child gets its own id and lives one hour. N is capped by the node's max_fork_count (default 16). All-or-nothing: on failure no child survives. The parent keeps running.", + "fork", "Clone a sandbox into N independent children that start from its exact memory and disk state, including running processes; each child gets its own id and lives one hour. N is capped by the node's max_fork_count (default 16). All-or-nothing: on failure no child survives. The parent keeps running. An egress-lane sandbox cannot fork.", schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint"), "count": integer("number of children, 1 to the node's max_fork_count (default 16)")}, "sandbox_id", "count"), toolFork, }, { - "checkpoint", "Capture a sandbox's full state (memory, disk, running processes) without stopping it and return a checkpoint id. branch_checkpoint claims new sandboxes from that moment any number of times; a checkpoint outlives this session until it is deleted or expires under the node's checkpoint TTL.", + "checkpoint", "Capture a sandbox's full state (memory, disk, running processes) without stopping it and return a checkpoint id. branch_checkpoint claims new sandboxes from that moment any number of times; a checkpoint outlives this session until it is deleted or expires under the node's checkpoint TTL. An egress-lane sandbox cannot checkpoint.", schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint"), "name": str("optional human label")}, "sandbox_id"), toolCheckpoint, }, { @@ -68,11 +68,11 @@ var ( {"list_checkpoints", "List the node's checkpoints newest first: checkpoint_id, name, source sandbox_id, created_at.", schema(props{}), toolListCheckpoints}, {"delete_checkpoint", "Delete the node's copy of a checkpoint and tell peers to drop theirs; a peer replica that misses that broadcast stays branchable until its own TTL sweep. Sandboxes already branched from it are unaffected.", schema(props{"checkpoint_id": str("id returned by checkpoint or list_checkpoints")}, "checkpoint_id"), toolDeleteCheckpoint}, { - "hibernate", "Snapshot a sandbox and stop its VM, freeing memory while keeping its id, files, processes, and shell state. The next call that reaches the guest (exec, spawn, ps, kill, logs, the file tools) wakes it transparently in tens of milliseconds; fork, checkpoint, promote and release act on the snapshot without waking it.", + "hibernate", "Snapshot a sandbox and stop its VM, freeing memory while keeping its id, files, processes, and shell state. The next call that reaches the guest (exec, spawn, ps, kill, logs, the file tools) wakes it transparently in tens of milliseconds; fork, checkpoint, promote and release act on the snapshot without waking it. An egress-lane sandbox cannot hibernate.", schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint")}, "sandbox_id"), toolHibernate, }, { - "promote", "Publish the sandbox's current state as a named template on its node; later create_sandbox calls with that name (and the same net and size) start from it. Re-promoting to the same name replaces the template.", + "promote", "Publish the sandbox's current state as a named template on its node; later create_sandbox calls with that name (and the same net and size) start from it. Re-promoting to the same name replaces the template. An egress-lane sandbox cannot promote.", schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint"), "template_name": str("template name to publish as")}, "sandbox_id", "template_name"), toolPromote, }, {"release", "Destroy a sandbox and free its resources; files and processes inside it are lost. This session forgets the id, so a second release of it is rejected as unknown.", schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint")}, "sandbox_id"), toolRelease}, @@ -109,7 +109,7 @@ func toolCreateSandbox(ctx context.Context, s *server, raw jsontext.Value) (stri if args.TTLSeconds < 0 { return "", fmt.Errorf("ttl_seconds must not be negative, got %d", args.TTLSeconds) } - // an agent session outlives the node's 5-minute default, and nothing renews a lease + // An agent session outlives the node's 5-minute default, and nothing renews a lease ttl := cmp.Or(time.Duration(args.TTLSeconds)*time.Second, defaultToolTTL) opts := []sandbox.Option{sandbox.WithTimeout(ttl)} if args.Net != "" { diff --git a/sandboxd/archives/pg/pg.go b/sandboxd/archives/pg/pg.go index eab199e2c..15cf03fb7 100644 --- a/sandboxd/archives/pg/pg.go +++ b/sandboxd/archives/pg/pg.go @@ -52,24 +52,14 @@ func Open(ctx context.Context, dsn, cell string) (*Registry, error) { } func (r *Registry) Put(ctx context.Context, rec archives.Record) (int64, error) { - qctx, cancel := context.WithTimeout(ctx, metastore.QueryTimeout) - defer cancel() - var epoch int64 - err := r.pool.QueryRow(qctx, `INSERT INTO sandboxd_archives (id, tenant, token_sha256, owner, epoch, checkpoint, deadline, claim, cell) + return r.advance(ctx, `INSERT INTO sandboxd_archives (id, tenant, token_sha256, owner, epoch, checkpoint, deadline, claim, cell) VALUES ($1, $2, $3, $4, nextval('sandboxd_archives_epoch'), $5, $6, $7, $8) ON CONFLICT (cell, id) DO UPDATE SET tenant = EXCLUDED.tenant, token_sha256 = EXCLUDED.token_sha256, epoch = EXCLUDED.epoch, consumed = false, checkpoint = EXCLUDED.checkpoint, deadline = EXCLUDED.deadline, claim = EXCLUDED.claim, updated_at = now() WHERE sandboxd_archives.owner = EXCLUDED.owner RETURNING epoch`, - rec.ID, rec.Tenant, rec.TokenSHA256, rec.Owner, rec.Checkpoint, nullTime(rec.Deadline), []byte(rec.Claim), r.cell).Scan(&epoch) - switch { - case errors.Is(err, pgx.ErrNoRows): - return 0, archives.ErrLost - case err != nil: - return 0, unavailable(err) - } - return epoch, nil + rec.ID, rec.Tenant, rec.TokenSHA256, rec.Owner, rec.Checkpoint, nullTime(rec.Deadline), []byte(rec.Claim), r.cell) } func (r *Registry) Replace(ctx context.Context, rec archives.Record, expect int64) (int64, error) { diff --git a/sandboxd/archives/pg/pg_test.go b/sandboxd/archives/pg/pg_test.go index 5d7ae2867..b3fe52721 100644 --- a/sandboxd/archives/pg/pg_test.go +++ b/sandboxd/archives/pg/pg_test.go @@ -47,7 +47,7 @@ func TestCellsKeepTheirOwnRows(t *testing.T) { } } -func TestSchemaIndexesTheCellDeadline(t *testing.T) { +func TestSchemaIndexesDeadlineAndConsumed(t *testing.T) { r := open(t, metastoretest.PGSchema(t), "c1") var n int if err := r.pool.QueryRow(t.Context(), `SELECT count(*) FROM pg_indexes diff --git a/sandboxd/config/config_test.go b/sandboxd/config/config_test.go index 3fbf15166..f1750c119 100644 --- a/sandboxd/config/config_test.go +++ b/sandboxd/config/config_test.go @@ -101,10 +101,11 @@ func TestAutoRefillConcurrency(t *testing.T) { } func TestLoadRejectsInvalid(t *testing.T) { + t.Setenv("SANDBOX_TEST_DSN", "postgres://sandboxd@db/sandboxd") for _, tt := range []struct { name, body, want string }{ - {"bad json", `{`, "config"}, + {"bad json", `{`, "parse config"}, {"bad fork count", `{"max_fork_count":-1,"pools":[]}`, "max_fork_count"}, {"egress conn rate without a rate", `{"egress_conn_rate":{"per_second":0,"burst":10},"pools":[]}`, "egress_conn_rate"}, {"egress conn rate without a burst", `{"egress_conn_rate":{"per_second":30},"pools":[]}`, "egress_conn_rate"}, @@ -130,7 +131,7 @@ func TestLoadRejectsInvalid(t *testing.T) { {"pool egress unknown secret", `{"pools":[{"template":"rt:24.04","net":"none","size":"small","egress":{"allow":[{"host":"api.github.com","secret":"gh"}]}}]}`, "unknown secret"}, {"meta_store unknown kind", `{"api_token":"r","pools":[],"meta_store":{"kind":"redis","dsn_env":"HOME"}}`, "not supported"}, {"meta_store dsn env unset", `{"api_token":"r","pools":[],"meta_store":{"kind":"pg","dsn_env":"SANDBOX_TEST_UNSET_DSN"}}`, "must name a set node env"}, - {"meta_store bad cell", `{"api_token":"r","pools":[],"meta_store":{"kind":"pg","dsn_env":"HOME","cell":"Cell A"}}`, "meta_store.cell"}, + {"meta_store bad cell", `{"api_token":"r","pools":[],"meta_store":{"kind":"pg","dsn_env":"SANDBOX_TEST_DSN","cell":"Cell A"}}`, "meta_store.cell"}, {"egress class unknown secret", `{"pools":[],"egress_classes":[{"name":"desk","egress":{"allow":[{"host":"x","secret":"gh"}]}}]}`, "unknown secret"}, {"egress class intercepts", `{"pools":[],"egress_classes":[{"name":"desk","egress":{"allow":[{"host":"x","intercept":true}]}}]}`, "only be set on a pool rule"}, {"egress class without egress", `{"pools":[],"egress_classes":[{"name":"desk"}]}`, "needs egress"}, diff --git a/sandboxd/egress/bench_test.go b/sandboxd/egress/bench_test.go index 72879ddbb..125ee47ee 100644 --- a/sandboxd/egress/bench_test.go +++ b/sandboxd/egress/bench_test.go @@ -214,4 +214,7 @@ func benchRoundTrip(b *testing.B, tc *tls.Conn, br *bufio.Reader) { } _, _ = io.Copy(io.Discard, resp.Body) _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK { + b.Fatalf("status %d, want 200", resp.StatusCode) + } } diff --git a/sandboxd/egress/ca.go b/sandboxd/egress/ca.go index bced95d2b..fc6e414c6 100644 --- a/sandboxd/egress/ca.go +++ b/sandboxd/egress/ca.go @@ -57,7 +57,7 @@ func LoadCA(rootCertPEM, interCertPEM, interKeyPEM []byte) (*CA, error) { if err = inter.CheckSignatureFrom(root); err != nil { return nil, fmt.Errorf("intermediate not signed by root: %w", err) } - // reject a self-signed intermediate: it would put the root key on the node. + // Reject a self-signed intermediate: it would put the root key on the node. if inter.CheckSignatureFrom(inter) == nil { return nil, fmt.Errorf("intermediate must not be a self-signed root") } @@ -68,7 +68,7 @@ func LoadCA(rootCertPEM, interCertPEM, interKeyPEM []byte) (*CA, error) { if err != nil { return nil, fmt.Errorf("generate leaf key: %w", err) } - // fingerprint the whole file: CertPEM bakes it verbatim. + // Fingerprint the whole file: CertPEM bakes it verbatim. sum := sha256.Sum256(rootCertPEM) return &CA{ rootPEM: rootCertPEM, @@ -93,7 +93,7 @@ func (c *CA) SignLeaf(host string) (*tls.Certificate, error) { return nil, err } now := time.Now() - // guests reject a chain whose leaf outlives its intermediate. + // Guests reject a chain whose leaf outlives its intermediate. notAfter := now.Add(leafValidity) if notAfter.After(c.interCert.NotAfter) { notAfter = c.interCert.NotAfter diff --git a/sandboxd/egress/intercept.go b/sandboxd/egress/intercept.go index 787197fb0..5124fb981 100644 --- a/sandboxd/egress/intercept.go +++ b/sandboxd/egress/intercept.go @@ -48,7 +48,7 @@ func (p *Proxy) serveIntercept(w http.ResponseWriter, r *http.Request, host stri if _, err := io.WriteString(client, "HTTP/1.1 200 Connection Established\r\n\r\n"); err != nil { return } - // a guest that never sends a ClientHello would pin this goroutine until the sandbox dies. + // A guest that never sends a ClientHello would pin this goroutine until the sandbox dies. _ = client.SetDeadline(time.Now().Add(interceptTimeout)) tlsConn := tls.Server(utils.WithBuffered(client, brw.Reader), cfg) if err := tlsConn.HandshakeContext(r.Context()); err != nil { @@ -67,7 +67,7 @@ func (p *Proxy) serveIntercept(w http.ResponseWriter, r *http.Request, host stri func (p *Proxy) leafFor(host string) (*tls.Certificate, error) { p.leafMu.Lock() defer p.leafMu.Unlock() - // a leaf clamped to the intermediate's own end has nothing to renew into + // A leaf clamped to the intermediate's own end has nothing to renew into if crt, ok := p.leaves[host]; ok && (time.Now().Before(crt.Leaf.NotAfter.Add(-leafRenewBefore)) || !crt.Leaf.NotAfter.Before(p.ca.interCert.NotAfter)) { return crt, nil } @@ -97,7 +97,7 @@ func (h *interceptHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { p.relay(w, r, ev, rule, route, true, func(out *http.Request) { out.URL.Scheme = "https" out.URL.Host = h.authority - // keep the guest's Host when it names the CONNECT host: SigV4 signing breaks on a rewrite. + // Keep the guest's Host when it names the CONNECT host: SigV4 signing breaks on a rewrite. if inner, _, _ := hostPort(r.Host, 0); !strings.EqualFold(inner, h.host) { out.Host = h.authority } diff --git a/sandboxd/egress/intercept_test.go b/sandboxd/egress/intercept_test.go index 24827d0df..cdbb239e7 100644 --- a/sandboxd/egress/intercept_test.go +++ b/sandboxd/egress/intercept_test.go @@ -129,7 +129,7 @@ func TestInterceptBindsInnerRequestToInterceptRule(t *testing.T) { tc := connectTLS(t, front.Listener.Addr().String(), "api.example.com:443", "api.example.com", guestRoots) defer func() { _ = tc.Close() }() - resp := roundTripTLSTo(t, tc, http.MethodGet, "api.example.com") + resp := roundTripTLSHost(t, tc, http.MethodGet, "api.example.com") defer func() { _ = resp.Body.Close() }() if seen := resp.Header.Get("X-Auth-Seen"); seen != "Bearer SECRET" { @@ -137,43 +137,41 @@ func TestInterceptBindsInnerRequestToInterceptRule(t *testing.T) { } } -func TestInterceptCaptureEnforcesRuleMethod(t *testing.T) { - policy := Policy{Allow: []Rule{ - {Host: "*.example.com"}, - {Host: "api.example.com", Methods: []string{"GET"}, Intercept: InterceptAlways}, - }} - p, guestRoots, upstream := interceptProxyPolicy(t, policy, nil) - p.pools.Load().mitm.TLSClientConfig.RootCAs = trustUpstream(upstream) - front := httptest.NewServer(p) - defer front.Close() - - tc := connectTLS(t, front.Listener.Addr().String(), "api.example.com:443", "api.example.com", guestRoots) - defer func() { _ = tc.Close() }() - resp := roundTripTLSTo(t, tc, http.MethodPost, "api.example.com") - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusForbidden { - t.Errorf("POST over the intercepted tunnel got %d, want 403 (intercept rule is GET-only)", resp.StatusCode) - } -} - -func TestInterceptReachesLaterInterceptRuleByMethod(t *testing.T) { - policy := Policy{Allow: []Rule{ - {Host: "*.example.com", Methods: []string{"GET"}, Intercept: InterceptAlways}, - {Host: "api.example.com", Methods: []string{"POST"}, Intercept: InterceptAlways}, - }} - p, guestRoots, upstream := interceptProxyPolicy(t, policy, nil) - p.pools.Load().mitm.TLSClientConfig.RootCAs = trustUpstream(upstream) - front := httptest.NewServer(p) - defer front.Close() - - tc := connectTLS(t, front.Listener.Addr().String(), "api.example.com:443", "api.example.com", guestRoots) - defer func() { _ = tc.Close() }() - resp := roundTripTLSTo(t, tc, http.MethodPost, "api.example.com") - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - t.Errorf("POST got %d, want 200 (the POST-only intercept rule must be reachable)", resp.StatusCode) +func TestInterceptEnforcesRuleMethods(t *testing.T) { + for _, tt := range []struct { + name string + policy Policy + host string + method string + want int + }{ + {"a capture rule keeps its method filter", Policy{Allow: []Rule{ + {Host: "*.example.com"}, + {Host: "api.example.com", Methods: []string{"GET"}, Intercept: InterceptAlways}, + }}, "api.example.com", http.MethodPost, http.StatusForbidden}, + {"a later intercept rule is reached by its method", Policy{Allow: []Rule{ + {Host: "*.example.com", Methods: []string{"GET"}, Intercept: InterceptAlways}, + {Host: "api.example.com", Methods: []string{"POST"}, Intercept: InterceptAlways}, + }}, "api.example.com", http.MethodPost, http.StatusOK}, + {"a method outside the rule is filtered", Policy{Allow: []Rule{ + {Host: "example.com", Methods: []string{"GET"}, Intercept: InterceptAlways}, + }}, "example.com", http.MethodPost, http.StatusForbidden}, + } { + t.Run(tt.name, func(t *testing.T) { + p, guestRoots, upstream := interceptProxyPolicy(t, tt.policy, nil) + p.pools.Load().mitm.TLSClientConfig.RootCAs = trustUpstream(upstream) + front := httptest.NewServer(p) + defer front.Close() + + tc := connectTLS(t, front.Listener.Addr().String(), tt.host+":443", tt.host, guestRoots) + defer func() { _ = tc.Close() }() + resp := roundTripTLSHost(t, tc, tt.method, tt.host) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != tt.want { + t.Errorf("%s over the intercepted tunnel got %d, want %d", tt.method, resp.StatusCode, tt.want) + } + }) } } @@ -219,22 +217,6 @@ func TestCloseRevokesInterceptedSession(t *testing.T) { } } -func TestInterceptFiltersByMethod(t *testing.T) { - p, guestRoots, upstream := interceptProxy(t, Rule{Host: "example.com", Methods: []string{"GET"}, Intercept: InterceptAlways}, nil) - p.pools.Load().mitm.TLSClientConfig.RootCAs = trustUpstream(upstream) - front := httptest.NewServer(p) - defer front.Close() - - tc := connectTLS(t, front.Listener.Addr().String(), "example.com:443", "example.com", guestRoots) - defer func() { _ = tc.Close() }() - resp := roundTripTLS(t, tc, http.MethodPost) - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusForbidden { - t.Errorf("POST over intercepted TLS got %d, want 403 (method filtered)", resp.StatusCode) - } -} - func TestInterceptRejectsUntrustedUpstream(t *testing.T) { p, guestRoots, _ := interceptProxy(t, Rule{Host: "example.com", Intercept: InterceptAlways}, nil) front := httptest.NewServer(p) @@ -702,22 +684,6 @@ func roundTripTLS(t *testing.T, tc *tls.Conn, method string) *http.Response { return roundTripTLSHost(t, tc, method, "") } -func roundTripTLSTo(t *testing.T, tc *tls.Conn, method, host string) *http.Response { - t.Helper() - req, err := http.NewRequestWithContext(t.Context(), method, "https://"+host+"/x", nil) - if err != nil { - t.Fatalf("build request: %v", err) - } - if err = req.Write(tc); err != nil { - t.Fatalf("write request: %v", err) - } - resp, err := http.ReadResponse(bufio.NewReader(tc), req) - if err != nil { - t.Fatalf("read response: %v", err) - } - return resp -} - func roundTripTLSHost(t *testing.T, tc *tls.Conn, method, host string) *http.Response { t.Helper() req, err := http.NewRequestWithContext(t.Context(), method, "https://example.com/x", nil) diff --git a/sandboxd/egress/policy_test.go b/sandboxd/egress/policy_test.go index 7ff18b55b..d892c69cd 100644 --- a/sandboxd/egress/policy_test.go +++ b/sandboxd/egress/policy_test.go @@ -22,7 +22,7 @@ func TestPolicyEval(t *testing.T) { {"exact host case-insensitive", "API.GitHub.com", "POST", DecisionAllow, "gh"}, {"exact host wrong method falls through to catch-all deny", "api.github.com", "DELETE", DecisionDeny, ""}, {"wildcard subdomain", "storage.googleapis.com", "PUT", DecisionAllow, ""}, - {"wildcard does not match apex", "googleapis.com", "GET", DecisionAllow, ""}, + {"wildcard does not match apex", "googleapis.com", "PUT", DecisionDeny, ""}, {"catch-all get", "example.com", "GET", DecisionAllow, ""}, {"catch-all denies non-get", "example.com", "POST", DecisionDeny, ""}, } diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index d99fc8812..cf7f18737 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -170,7 +170,7 @@ func New(policy Evaluator, secrets Secrets, ca *CA, router Router, audit AuditFu holder: holder, conns: map[net.Conn]struct{}{}, } - // the stdlib default of 2 idle conns per host re-dials bursty same-host traffic. + // The stdlib default of 2 idle conns per host re-dials bursty same-host traffic. pools := &connPools{tr: &http.Transport{DialContext: direct, MaxIdleConns: maxIdleConns, MaxIdleConnsPerHost: 8, IdleConnTimeout: idleConnTimeout}, routed: map[routeKey]*http.Transport{}} if ca != nil { pools.mitm = pools.tr.Clone() @@ -314,7 +314,7 @@ func (p *Proxy) serveConnect(w http.ResponseWriter, r *http.Request) { // tunnelDecision treats an inject rule as a plain one when injects is false. func (p *Proxy) tunnelDecision(host string, port uint16, injects bool) (decision Decision, intercept bool) { - // host-gate interception: the tunnel's CONNECT verb is not the request method. + // Host-gate interception: the tunnel's CONNECT verb is not the request method. if p.ca != nil { if rule, d := p.policy.EvalHost(host, port); d == DecisionAllow && (rule.Intercept == InterceptAlways || rule.Intercept == InterceptInject && injects && p.holdsCredential(host)) { @@ -374,7 +374,7 @@ func (p *Proxy) relay(w http.ResponseWriter, r *http.Request, ev Event, rule Rul var sc *scrubber ev.Injected, sc = p.inject(rule, out, ev.Host, mitm) if sc != nil { - // the scrub needs the whole body uncompressed. + // The scrub needs the whole body uncompressed. for _, h := range []string{"Accept-Encoding", "Range", "If-Range"} { out.Header.Del(h) } diff --git a/sandboxd/egress/proxy_test.go b/sandboxd/egress/proxy_test.go index 41c40ccaf..485dedc1f 100644 --- a/sandboxd/egress/proxy_test.go +++ b/sandboxd/egress/proxy_test.go @@ -585,11 +585,17 @@ func TestAuditNamesTheConnectionEnds(t *testing.T) { var d net.Dialer return d.DialContext(ctx, network, target) } - p := New(Policy{Allow: []Rule{{Host: "*"}}}, nil, nil, DialFunc(dial), func(ev Event) { events <- ev }, nil) - front := httptest.NewServer(p) - defer front.Close() + serve := func(recordEnds bool) *httptest.Server { + p := New(Policy{Allow: []Rule{{Host: "*"}}}, nil, nil, DialFunc(dial), func(ev Event) { events <- ev }, nil) + if recordEnds { + p.RecordEnds() + } + front := httptest.NewServer(p) + t.Cleanup(front.Close) + return front + } - resp, err := proxyClient(t, front.URL).Get("http://api.internal/x") + resp, err := proxyClient(t, serve(false).URL).Get("http://api.internal/x") if err != nil { t.Fatalf("proxied GET: %v", err) } @@ -597,8 +603,8 @@ func TestAuditNamesTheConnectionEnds(t *testing.T) { if ev := recvEvent(t, events); ev.Local.IsValid() || ev.Remote.IsValid() { t.Errorf("forward event %+v carries ends without RecordEnds", ev) } - p.RecordEnds() + front := serve(true) resp, err = proxyClient(t, front.URL).Get("http://api.internal/y") if err != nil { t.Fatalf("proxied GET: %v", err) diff --git a/sandboxd/egress/splice_linux_test.go b/sandboxd/egress/splice_linux_test.go index 93ce32fad..4af97874f 100644 --- a/sandboxd/egress/splice_linux_test.go +++ b/sandboxd/egress/splice_linux_test.go @@ -33,12 +33,13 @@ func testSpliceThroughDoor(t *testing.T, noSplice bool) { t.Fatalf("listen tcp: %v", err) } t.Cleanup(func() { _ = tln.Close() }) + buffered := make(chan struct{}, 2) go func() { c, acceptErr := tln.Accept() if acceptErr != nil { return } - pump(c, total) + pump(c, total, buffered) _ = c.(*net.TCPConn).CloseWrite() }() guestDone := make(chan struct{}) @@ -50,6 +51,7 @@ func testSpliceThroughDoor(t *testing.T, noSplice bool) { } defer func() { _ = c.Close() }() buf := make([]byte, 64<<10) + buffered <- struct{}{} for n := 0; n < total; n += len(buf) { _, _ = c.Write(buf) } @@ -71,6 +73,8 @@ func testSpliceThroughDoor(t *testing.T, noSplice bool) { door := doorShaped{guest.(*net.UnixConn)} defer func() { _ = door.Close(); _ = upstream.Close() }() + <-buffered + <-buffered runtime.GC() var before, after runtime.MemStats runtime.ReadMemStats(&before) @@ -89,8 +93,9 @@ type doorShaped struct{ *net.UnixConn } func (c doorShaped) NetConn() net.Conn { return c.UnixConn } -func pump(c net.Conn, total int) { +func pump(c net.Conn, total int, buffered chan<- struct{}) { buf := make([]byte, 64<<10) + buffered <- struct{}{} for n := 0; n < total; { m, err := c.Read(buf) n += m diff --git a/sandboxd/egress/upstream_test.go b/sandboxd/egress/upstream_test.go index aad7c6c09..6ad555cd9 100644 --- a/sandboxd/egress/upstream_test.go +++ b/sandboxd/egress/upstream_test.go @@ -389,7 +389,6 @@ func assertEcho(t *testing.T, conn net.Conn) { } } -// fakeConnectProxy answers one CONNECT with status and, on 200, echoes the tunnel. func fakeConnectProxy(t *testing.T, status int, greeting string) (string, <-chan string) { t.Helper() ln, err := net.Listen("tcp", "127.0.0.1:0") @@ -418,7 +417,6 @@ func fakeConnectProxy(t *testing.T, status int, greeting string) (string, <-chan return ln.Addr().String(), seen } -// fakeSOCKS5 accepts one user/password CONNECT and echoes the tunnel. func fakeSOCKS5(t *testing.T) (string, <-chan string) { t.Helper() ln, err := net.Listen("tcp", "127.0.0.1:0") diff --git a/sandboxd/engine/installca.go b/sandboxd/engine/installca.go index da0d42791..4c0da7717 100644 --- a/sandboxd/engine/installca.go +++ b/sandboxd/engine/installca.go @@ -46,7 +46,7 @@ func (e *Engine) silkdWriteFile(ctx context.Context, vsockSocket, path string, m return err } defer s.close() - // a send racing the guest's early answer+close hits EPIPE; prefer the buffered verdict. + // A send racing the guest's early answer+close hits EPIPE; prefer the buffered verdict. sendErr := func() error { if serr := s.send(wire.FsWrite{Path: path, Mode: &mode}); serr != nil { return serr @@ -85,7 +85,7 @@ func (e *Engine) silkdOutput(ctx context.Context, vsockSocket string, argv ...st defer s.close() sendErr := s.send(wire.Exec{Argv: argv, Env: map[string]string{"PATH": guestExecPATH}}) if sendErr == nil { - // a child exiting without reading stdin races this close; prefer the buffered exit frame. + // A child exiting without reading stdin races this close; prefer the buffered exit frame. sendErr = s.send(wire.StdinClose{}) } var out, stdout []byte diff --git a/sandboxd/engine/installca_test.go b/sandboxd/engine/installca_test.go index 1d79d029d..a890f591e 100644 --- a/sandboxd/engine/installca_test.go +++ b/sandboxd/engine/installca_test.go @@ -89,6 +89,7 @@ type fakeSilkd struct { block []wire.DirEntry serial map[string][]byte readMisses map[string]int + blankReads map[string]int listErr string readErr map[string]string listCalls int @@ -113,6 +114,7 @@ func serveFakeSilkd(t *testing.T, path string) *fakeSilkd { f := &fakeSilkd{ serial: make(map[string][]byte), readMisses: make(map[string]int), + blankReads: make(map[string]int), readErr: make(map[string]string), stat: make(map[string]bool), statMisses: make(map[string]int), @@ -277,6 +279,10 @@ func (f *fakeSilkd) handleRead(conn net.Conn, path string) { f.readMisses[path]-- ok = false } + if f.blankReads[path] > 0 { + f.blankReads[path]-- + data = []byte("\n") + } data = bytes.Clone(data) f.mu.Unlock() if reply != "" { diff --git a/sandboxd/engine/portconn.go b/sandboxd/engine/portconn.go index 053121515..45c662396 100644 --- a/sandboxd/engine/portconn.go +++ b/sandboxd/engine/portconn.go @@ -73,7 +73,7 @@ func (g *guestPortConn) Write(p []byte) (int, error) { written := 0 for len(p) > 0 { n := min(len(p), wire.PortWriteChunk) - // the hot relay path reuses one buffer instead of allocating per chunk. + // The hot relay path reuses one buffer instead of allocating per chunk. g.wbuf = wire.AppendBulkRequest(g.wbuf, "data", p[:n]) if _, err := g.Conn.Write(g.wbuf); err != nil { return written, err diff --git a/sandboxd/engine/reader.go b/sandboxd/engine/reader.go index 10f5df793..d81d55880 100644 --- a/sandboxd/engine/reader.go +++ b/sandboxd/engine/reader.go @@ -55,7 +55,8 @@ func (e *Engine) StartReader(ctx context.Context, vsockSocket, cowDisk string) e if err = e.silkdExec(ctx, vsockSocket, "test", "-x", readerView+readerSilkd); err != nil { return fmt.Errorf("claim image has no silkd at %s: %w", readerSilkd, err) } - if err = e.silkdSpawn(ctx, vsockSocket, "env", fmt.Sprintf("SILKD_PORT=%d", readerPort), "chroot", readerView, readerSilkd); err != nil { + argv := []string{"env", fmt.Sprintf("SILKD_PORT=%d", readerPort), "chroot", readerView, readerSilkd} + if err = e.silkdOnce[*wire.Started](ctx, vsockSocket, wire.Exec{Argv: argv, Env: map[string]string{"PATH": guestExecPATH}, Detach: true}); err != nil { return fmt.Errorf("start view silkd: %w", err) } return e.probe(ctx, vsockSocket, readerPort, readerProbeTimeout) @@ -66,29 +67,6 @@ func (e *Engine) DialReader(ctx context.Context, vsockSocket string) (net.Conn, return e.dialVsock(ctx, vsockSocket, readerPort) } -func (e *Engine) silkdSpawn(ctx context.Context, vsockSocket string, argv ...string) error { - s, err := e.dialSilkdSession(ctx, vsockSocket) - if err != nil { - return err - } - defer s.close() - if err = s.send(wire.Exec{Argv: argv, Env: map[string]string{"PATH": guestExecPATH}, Detach: true}); err != nil { - return err - } - frame, err := s.recv() - if err != nil { - return err - } - switch resp := frame.(type) { - case *wire.Started: - return nil - case *wire.ErrorResp: - return fmt.Errorf("silkd %w", resp) - default: - return fmt.Errorf("unexpected silkd frame %q", frame.RespType()) - } -} - func cmdlineLayers(cmdline string) []string { var layers []string for field := range strings.FieldsSeq(cmdline) { diff --git a/sandboxd/engine/silkd.go b/sandboxd/engine/silkd.go index d7bfab3d1..211f7fd93 100644 --- a/sandboxd/engine/silkd.go +++ b/sandboxd/engine/silkd.go @@ -127,26 +127,25 @@ func (e *Engine) silkdReadFile(ctx context.Context, vsockSocket, path string, li return data, nil } -// silkdStat answers with a single Stat frame and no Done, so it cannot ride silkdStream. -func (e *Engine) silkdStat(ctx context.Context, vsockSocket, path string) error { +// silkdOnce sends a request answered by a single T frame and no Done, so it cannot ride silkdStream. +func (e *Engine) silkdOnce[T wire.Response](ctx context.Context, vsockSocket string, req wire.Request) error { s, err := e.dialSilkdSession(ctx, vsockSocket) if err != nil { return err } defer s.close() - if err = s.send(wire.FsStat{Path: path}); err != nil { + if err = s.send(req); err != nil { return err } frame, err := s.recv() if err != nil { return err } - switch resp := frame.(type) { - case *wire.Stat: + if _, ok := frame.(T); ok { return nil - case *wire.ErrorResp: + } + if resp, ok := frame.(*wire.ErrorResp); ok { return fmt.Errorf("silkd %w", resp) - default: - return fmt.Errorf("unexpected silkd frame %q", frame.RespType()) } + return fmt.Errorf("unexpected silkd frame %q", frame.RespType()) } diff --git a/sandboxd/engine/volume.go b/sandboxd/engine/volume.go index 47a1600e7..e7ba47dff 100644 --- a/sandboxd/engine/volume.go +++ b/sandboxd/engine/volume.go @@ -93,8 +93,7 @@ func (e *Engine) umountVolume(ctx context.Context, vsockSocket, mount string) er func (e *Engine) waitForVolumeDevice(ctx context.Context, vsockSocket, name string) (string, error) { ctx, cancel := context.WithTimeout(ctx, volumeProbeTimeout) defer cancel() - ticker := time.NewTicker(volumePollInterval) - defer ticker.Stop() + tick := time.Tick(volumePollInterval) others := map[string]bool{} for { device, found, err := e.findVolumeDevice(ctx, vsockSocket, name, others) @@ -107,7 +106,7 @@ func (e *Engine) waitForVolumeDevice(ctx context.Context, vsockSocket, name stri select { case <-ctx.Done(): return "", ctx.Err() - case <-ticker.C: + case <-tick: } } } @@ -133,8 +132,8 @@ func (e *Engine) findVolumeDevice(ctx context.Context, vsockSocket, name string, continue } device := "/dev/" + entry.Name - // the kernel publishes /sys/block before devtmpfs creates the node. - if err := e.silkdStat(ctx, vsockSocket, device); err != nil { + // The kernel publishes /sys/block before devtmpfs creates the node. + if err := e.silkdOnce[*wire.Stat](ctx, vsockSocket, wire.FsStat{Path: device}); err != nil { if isNotFound(err) { return "", false, nil } @@ -149,8 +148,7 @@ func (e *Engine) mountDevice(ctx context.Context, vsockSocket, device, name, mou if err := e.silkdExec(ctx, vsockSocket, "mkdir", "-p", "--", mount); err != nil { return fmt.Errorf("create volume mount point %s: %w", mount, err) } - ticker := time.NewTicker(volumePollInterval) - defer ticker.Stop() + tick := time.Tick(volumePollInterval) asExt4 := true for { err := e.silkdExec(ctx, vsockSocket, mountArgv(asExt4, mountMode(rw), device, mount)...) @@ -170,7 +168,7 @@ func (e *Engine) mountDevice(ctx context.Context, vsockSocket, device, name, mou select { case <-ctx.Done(): return fmt.Errorf("mount volume %s: %w: %w", name, ctx.Err(), err) - case <-ticker.C: + case <-tick: } } } diff --git a/sandboxd/engine/volume_test.go b/sandboxd/engine/volume_test.go index b955bf5be..9e7d02f6a 100644 --- a/sandboxd/engine/volume_test.go +++ b/sandboxd/engine/volume_test.go @@ -282,27 +282,21 @@ func TestMountVolumeReadsEachOtherSerialOnce(t *testing.T) { fake := serveFakeSilkd(t, path) configureVolumeDevices(fake) fake.mu.Lock() - imagenet := fake.serial["/sys/block/vdc/serial"] - fake.serial["/sys/block/vdc/serial"] = []byte("\n") + fake.blankReads["/sys/block/vdc/serial"] = 3 fake.mu.Unlock() - time.AfterFunc(60*time.Millisecond, func() { - fake.mu.Lock() - defer fake.mu.Unlock() - fake.serial["/sys/block/vdc/serial"] = imagenet - }) if err := New("cocoon", nil, nil, false, false, "").MountVolume(t.Context(), path, "imagenet", "/datasets/training", false); err != nil { t.Fatalf("MountVolume: %v", err) } fake.mu.Lock() defer fake.mu.Unlock() - if fake.listCalls < 2 { - t.Fatalf("list calls = %d, want the device to arrive on a later poll", fake.listCalls) + if fake.listCalls != 4 { + t.Fatalf("list calls = %d, want 4 (3 blank serials + the successful poll)", fake.listCalls) } if got := len(slices.DeleteFunc(slices.Clone(fake.readCalls), func(p string) bool { return p != "/sys/block/vda/serial" })); got != 1 { t.Errorf("read the root disk's serial %d times, want once: %v", got, fake.readCalls) } - if got := len(slices.DeleteFunc(slices.Clone(fake.readCalls), func(p string) bool { return p != "/sys/block/vdc/serial" })); got < 2 { - t.Errorf("read the awaited disk's serial %d times, want a retry: %v", got, fake.readCalls) + if got := len(slices.DeleteFunc(slices.Clone(fake.readCalls), func(p string) bool { return p != "/sys/block/vdc/serial" })); got != 4 { + t.Errorf("read the awaited disk's serial %d times, want 4: %v", got, fake.readCalls) } } diff --git a/sandboxd/main.go b/sandboxd/main.go index 7e080d6da..c2bd40963 100644 --- a/sandboxd/main.go +++ b/sandboxd/main.go @@ -239,13 +239,12 @@ func startMesh(ctx context.Context, cfg *config.Config, mgr *pool.Manager) (*mes } func gossipNodeState(ctx context.Context, msh *mesh.Mesh, mgr *pool.Manager) { - t := time.NewTicker(gossipInterval) - defer t.Stop() + tick := time.Tick(gossipInterval) for { select { case <-ctx.Done(): return - case <-t.C: + case <-tick: msh.UpdateSelf(ctx, mgr.WarmCounts(), mgr.TemplateHashes(), mgr.VolumeNames()) } } diff --git a/sandboxd/outbound/dial.go b/sandboxd/outbound/dial.go index 54e9cefed..6a42dbb83 100644 --- a/sandboxd/outbound/dial.go +++ b/sandboxd/outbound/dial.go @@ -96,7 +96,7 @@ func newDialer(verdict VerdictFunc) *net.Dialer { // destVerdict reports whether ip is an internal address allow re-admits; any other internal address, self included, is an error. func destVerdict(allow []egress.InternalAllow, self []netip.Addr, ip netip.Addr, port uint16) (bool, error) { ip = canonicalAddr(ip) - // after the NAT64 unwrap and before the block, so a named prefix wins + // After the NAT64 unwrap and before the block, so a named prefix wins if slices.ContainsFunc(allow, func(a egress.InternalAllow) bool { return a.Admits(ip, port) }) { return true, nil } diff --git a/sandboxd/outbound/doors_test.go b/sandboxd/outbound/doors_test.go index 1c5cec9ae..48f304f63 100644 --- a/sandboxd/outbound/doors_test.go +++ b/sandboxd/outbound/doors_test.go @@ -6,7 +6,6 @@ import ( "net" "net/http" "net/http/httptest" - "os" "path/filepath" "syscall" "testing" @@ -50,12 +49,7 @@ func TestEgressDoorHalfCloseReachesTheGuest(t *testing.T) { } func TestDoorConnHandsSpliceItsUnixConn(t *testing.T) { - dir, err := os.MkdirTemp("/tmp", "door") - if err != nil { - t.Fatalf("temp dir: %v", err) - } - t.Cleanup(func() { _ = os.RemoveAll(dir) }) - ln, err := listenUnix(filepath.Join(dir, "d")) + ln, err := listenUnix(filepath.Join(outboundtest.SockRoot(t), "d")) if err != nil { t.Fatalf("listen: %v", err) } @@ -94,12 +88,13 @@ func TestEgressDoorCapsConcurrentConnections(t *testing.T) { if err := h.Arm(t.Context(), sb); err != nil { t.Fatalf("arm egress: %v", err) } + t.Cleanup(func() { h.Disarm(sb.ID, true) }) path := engine.EgressSocketPath(sb.VsockSocket) idle := make([]net.Conn, 0, doorConns) for len(idle) < doorConns { conn, err := net.Dial("unix", path) - if errors.Is(err, syscall.ECONNREFUSED) { + if errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.EAGAIN) { time.Sleep(10 * time.Millisecond) continue } @@ -114,16 +109,22 @@ func TestEgressDoorCapsConcurrentConnections(t *testing.T) { } }) + waitHeld(t, h, sb.ID, doorConns) + client := outboundtest.EgressClient(path) client.Timeout = 500 * time.Millisecond - if resp, err := client.Get(origin.URL + "/"); err == nil { + resp, err := client.Get(origin.URL + "/") + if err == nil { resp.Body.Close() t.Fatal("a request past the door cap was served") } + if netErr, ok := errors.AsType[net.Error](err); !ok || !netErr.Timeout() { + t.Fatalf("request past the door cap: %v, want it to wait out the client timeout", err) + } _ = idle[0].Close() client.Timeout = 5 * time.Second - resp, err := client.Get(origin.URL + "/") + resp, err = client.Get(origin.URL + "/") if err != nil { t.Fatalf("request after a slot freed: %v", err) } @@ -131,16 +132,10 @@ func TestEgressDoorCapsConcurrentConnections(t *testing.T) { if resp.StatusCode != http.StatusOK { t.Errorf("status %d after a slot freed, want 200", resp.StatusCode) } - h.Disarm(sb.ID, true) } func TestReachedSeesAGuestInTheBacklog(t *testing.T) { - dir, err := os.MkdirTemp("/tmp", "door") - if err != nil { - t.Fatalf("temp dir: %v", err) - } - t.Cleanup(func() { _ = os.RemoveAll(dir) }) - path := filepath.Join(dir, "d.sock") + path := filepath.Join(outboundtest.SockRoot(t), "d.sock") ln, err := listenUnix(path) if err != nil { t.Fatalf("listen: %v", err) @@ -162,3 +157,17 @@ func TestReachedSeesAGuestInTheBacklog(t *testing.T) { t.Error("a door whose guest hung up reads as unreached") } } + +func waitHeld(t *testing.T, h *Host, id string, want int) { + t.Helper() + h.mu.Lock() + door := h.doors[id].ln.(*doorListener) + h.mu.Unlock() + for range 500 { + if len(door.slots) >= want { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("door holds %d connections, want %d", len(door.slots), want) +} diff --git a/sandboxd/outbound/outbound.go b/sandboxd/outbound/outbound.go index 7b38b05fd..112ef45b9 100644 --- a/sandboxd/outbound/outbound.go +++ b/sandboxd/outbound/outbound.go @@ -115,7 +115,7 @@ func (h *Host) ArmProxy(ctx context.Context, sb *types.Sandbox) error { d := h.prebound[sb.VMName] delete(h.prebound, sb.VMName) h.mu.Unlock() - // only an intercepting pool pays for the TLS transport and leaf cache + // Only an intercepting pool pays for the TLS transport and leaf cache ca := h.o.CA if !v.Intercepts(sb.PolicyKey()) { ca = nil diff --git a/sandboxd/pool/archive.go b/sandboxd/pool/archive.go index 603b60015..99eaf39a2 100644 --- a/sandboxd/pool/archive.go +++ b/sandboxd/pool/archive.go @@ -80,7 +80,7 @@ func (m *Manager) archiveOnce(ctx context.Context) { } func (m *Manager) archive(ctx context.Context, sb *types.Sandbox) error { - // must finish even if the sweep ctx is canceled, or the record diverges from the store. + // Must finish even if the sweep ctx is canceled, or the record diverges from the store. ctx = context.WithoutCancel(ctx) defer m.untrack(m.archiving, sb.ID) m.mu.Lock() @@ -98,18 +98,18 @@ func (m *Manager) archive(ctx context.Context, sb *types.Sandbox) error { if err = m.checkArchiveRow(ctx, sb.ID); err != nil { return fmt.Errorf("archive %s: registry: %w", sb.ID, err) } - // pin the ck before Publish makes it listable, or a delete would strand the claim + // Pin the ck before Publish makes it listable, or a delete would strand the claim ckID := store.CheckpointID(randHex(8)) m.mu.Lock() m.pendingCks[ckID] = struct{}{} m.mu.Unlock() defer m.untrack(m.pendingCks, ckID) - // a hibernated source is copied from its wake image, so no VM starts. + // A hibernated source is copied from its wake image, so no VM starts. ck, srcSnap, err := m.publishCheckpoint(ctx, sb, ckID, "archive", sb.Tenant, true) if err != nil { return err } - // the claim must still hold the exact snapshot the export captured, or the ck is stale + // The claim must still hold the exact snapshot the export captured, or the ck is stale sb.Transition.Lock() m.mu.Lock() if m.claimed[sb.ID] != sb || sb.HibernateSnap != srcSnap || sb.ArchiveCk != "" { @@ -152,7 +152,7 @@ func (m *Manager) archive(ctx context.Context, sb *types.Sandbox) error { if saveErr := m.store.commit(js); saveErr != nil { m.mu.Lock() var rb claimSnapshot - // a Release that landed meanwhile already deleted the claim; re-setting it would resurrect it + // A Release that landed meanwhile already deleted the claim; re-setting it would resurrect it if m.claimed[sb.ID] == sb { sb.ArchiveCk, sb.ArchiveEpoch, sb.ArchivePending, sb.ReplicaCk = "", 0, prevPending, replica sb.HibernateSnap, sb.VsockSocket, sb.VMName = snap, sock, vmName @@ -167,10 +167,10 @@ func (m *Manager) archive(ctx context.Context, sb *types.Sandbox) error { } m.counters.archives.Add(1) m.recordUsage(ctx, usage.Event{Event: "archive", ID: sb.ID, Reference: ck.ID, Tenant: sb.Tenant, Deadline: sb.Deadline}) - // disarm under Transition so a wake right after the release is not clobbered by a late disarm + // Disarm under Transition so a wake right after the release is not clobbered by a late disarm m.out.Disarm(sb.ID, true) sb.Transition.Unlock() - // the store ck is authoritative from here + // The store ck is authoritative from here m.destroy(ctx, vmName) m.dropSnap(ctx, snap) if replica != "" { @@ -181,7 +181,7 @@ func (m *Manager) archive(ctx context.Context, sb *types.Sandbox) error { // Caller holds Transition. func (m *Manager) wakeArchived(ctx context.Context, sb *types.Sandbox) (string, error) { - // egress never archives; a corrupt archived egress claim must fail closed. + // Egress never archives; a corrupt archived egress claim must fail closed. if sb.Key.Net == types.NetEgress { return "", fmt.Errorf("wake %s: egress lane cannot resume from archive", sb.ID) } @@ -227,7 +227,7 @@ func (m *Manager) wakeArchived(ctx context.Context, sb *types.Sandbox) (string, return "", fmt.Errorf("wake %s: persist claims", sb.ID) } logger := log.WithFunc("pool.wakeArchived") - // the pre-commit marker keeps a failed delete retryable after the journal update. + // The pre-commit marker keeps a failed delete retryable after the journal update. if delErr := m.ckpts.Delete(ctx, ck); delErr != nil { logger.Warnf(ctx, "delete consumed archive ck %s: %v", ck, delErr) } else { @@ -236,7 +236,7 @@ func (m *Manager) wakeArchived(ctx context.Context, sb *types.Sandbox) (string, } m.dropTombstone(ctx, sb.ID, consumed) } - // only the none lane reaches here, so there is no NIC to re-lock + // Only the none lane reaches here, so there is no NIC to re-lock if proxyErr := m.out.ArmProxy(ctx, sb); proxyErr != nil { logger.Errorf(ctx, proxyErr, "arm egress proxy %s", sb.ID) } diff --git a/sandboxd/pool/capture.go b/sandboxd/pool/capture.go index bc10cec68..dbf0713ce 100644 --- a/sandboxd/pool/capture.go +++ b/sandboxd/pool/capture.go @@ -55,7 +55,7 @@ func (m *Manager) exportSource(ctx context.Context, sb *types.Sandbox, exportDir } defer cleanup() vmName, guestEnv = sb.VMName, m.heldGuestEnv(sb) - // a fresh snapshot is this capture's alone, so dials need not wait out its export; a wake consumes a hibernate snapshot + // A fresh snapshot is this capture's alone, so dials need not wait out its export; a wake consumes a hibernate snapshot if snap != sb.HibernateSnap { sb.Transition.Unlock() } else { diff --git a/sandboxd/pool/checkpoint.go b/sandboxd/pool/checkpoint.go index d80cd293e..8ec390c6a 100644 --- a/sandboxd/pool/checkpoint.go +++ b/sandboxd/pool/checkpoint.go @@ -60,7 +60,7 @@ func (m *Manager) Checkpoint(ctx context.Context, id string, cred Cred, name, te // ClaimCheckpoint provisions a fresh claim cloned from a checkpoint, attributed to tenant. func (m *Manager) ClaimCheckpoint(ctx context.Context, ckptID string, o ClaimOptions) (*types.Sandbox, error) { - // a rejected id must not leave a lock-map entry, so resolve before recLock and quota + // A rejected id must not leave a lock-map entry, so resolve before recLock and quota ckpt, err := m.loadCheckpoint(ctx, ckptID) if err != nil { return nil, err @@ -76,7 +76,7 @@ func (m *Manager) ClaimCheckpointHeal(ctx context.Context, ckptID string, o Clai if m.healer == nil || !store.CheckpointIDRe.MatchString(ckptID) { return nil, ErrUnknownCheckpoint } - // resolving moves a guest memory image, so quota rejects a full node before that cost + // Resolving moves a guest memory image, so quota rejects a full node before that cost if err := m.overQuota(1, o.Tenant); err != nil { return nil, err } @@ -130,7 +130,7 @@ func (m *Manager) DeleteCheckpoint(ctx context.Context, ckptID, tenant string, s if err := m.ckpts.Delete(ctx, ckptID); err != nil { return fmt.Errorf("delete checkpoint: %w", err) } - // a shared backend has no per-node replicas to chase. + // A shared backend has no per-node replicas to chase. if scope == DeleteFleet && m.peerDelete != nil && !m.ckptsShared { m.peerDelete(context.WithoutCancel(ctx), ckptID) } @@ -157,7 +157,7 @@ func (m *Manager) FetchCheckpoint(ctx context.Context, ckptID string) (string, [ } return "", nil, nil, fmt.Errorf("fetch checkpoint: %w", err) } - // the read lock spans the transfer so a delete cannot pull the export from under it + // The read lock spans the transfer so a delete cannot pull the export from under it return dir, meta, unlock, nil } @@ -384,14 +384,14 @@ func validateHealedCheckpoint(staging, wantID string) error { return fmt.Errorf("healed record %s has an invalid key: %w", wantID, keyErr) } if !ckpt.Key.Capturable() { - // publishing a non-branchable key would poison the id and suppress a good owner + // Publishing a non-branchable key would poison the id and suppress a good owner return fmt.Errorf("healed record %s has a non-branchable (egress) key", wantID) } export, err := os.ReadDir(filepath.Join(staging, store.ExportDir)) if err != nil { return fmt.Errorf("healed record %s missing export dir: %w", wantID, err) } - // an export with no regular file clones to nothing; deeper content is cocoon's own format + // An export with no regular file clones to nothing; deeper content is cocoon's own format if !hasRegularFile(export) { return fmt.Errorf("healed record %s has no export content", wantID) } diff --git a/sandboxd/pool/checkpoint_cleanup_test.go b/sandboxd/pool/checkpoint_cleanup_test.go index 907ef02b3..c94c71795 100644 --- a/sandboxd/pool/checkpoint_cleanup_test.go +++ b/sandboxd/pool/checkpoint_cleanup_test.go @@ -136,7 +136,11 @@ func TestCheckpointCleanupWaitsForTheClaimWrite(t *testing.T) { } } m.sweepOrphanArchives(t.Context()) - if rec, ok, _ := c.reg.Get(t.Context(), sb.ID); ok && rec.Consumed { + rec, ok, err := c.reg.Get(t.Context(), sb.ID) + if err != nil { + t.Fatalf("registry get: %v", err) + } + if ok && rec.Consumed { t.Error("sweep kept the tombstone after the claim write landed") } }) diff --git a/sandboxd/pool/checkpoint_heal_test.go b/sandboxd/pool/checkpoint_heal_test.go index 589b8260c..e49dd1b05 100644 --- a/sandboxd/pool/checkpoint_heal_test.go +++ b/sandboxd/pool/checkpoint_heal_test.go @@ -408,17 +408,7 @@ func (p *healPuller) Pull(_ context.Context, _, _, dst string) error { if p.release != nil { <-p.release } - if err := os.MkdirAll(filepath.Join(dst, store.ExportDir), 0o750); err != nil { - return err - } - if err := os.WriteFile(filepath.Join(dst, store.ExportDir, "mem"), []byte("state"), 0o600); err != nil { - return err - } - meta, err := json.Marshal(p.ckpt) - if err != nil { - return err - } - return os.WriteFile(filepath.Join(dst, store.MetaFile), meta, 0o600) + return writeHealedRecord(dst, p.ckpt) } func (p *healPuller) count() int { @@ -436,17 +426,7 @@ type blockingPuller struct { func (p *blockingPuller) Pull(_ context.Context, _, id, dst string) error { p.once.Do(func() { close(p.started) }) <-p.release - if err := os.MkdirAll(filepath.Join(dst, store.ExportDir), 0o750); err != nil { - return err - } - if err := os.WriteFile(filepath.Join(dst, store.ExportDir, "mem"), []byte("state"), 0o600); err != nil { - return err - } - meta, err := json.Marshal(types.Checkpoint{ID: id, Key: testKey, CreatedAt: time.Now()}) - if err != nil { - return err - } - return os.WriteFile(filepath.Join(dst, store.MetaFile), meta, 0o600) + return writeHealedRecord(dst, types.Checkpoint{ID: id, Key: testKey, CreatedAt: time.Now()}) } func plantHealedRecord(t *testing.T, dir string, ckpt types.Checkpoint) { @@ -454,19 +434,23 @@ func plantHealedRecord(t *testing.T, dir string, ckpt types.Checkpoint) { if ckpt.Key == (types.PoolKey{}) { ckpt.Key = testKey } - if err := os.MkdirAll(filepath.Join(dir, store.ExportDir), 0o750); err != nil { + if err := writeHealedRecord(dir, ckpt); err != nil { t.Fatalf("setup: %v", err) } +} + +func writeHealedRecord(dir string, ckpt types.Checkpoint) error { + if err := os.MkdirAll(filepath.Join(dir, store.ExportDir), 0o750); err != nil { + return err + } if err := os.WriteFile(filepath.Join(dir, store.ExportDir, "mem"), []byte("state"), 0o600); err != nil { - t.Fatalf("setup: %v", err) + return err } meta, err := json.Marshal(ckpt) if err != nil { - t.Fatalf("setup: %v", err) - } - if err := os.WriteFile(filepath.Join(dir, store.MetaFile), meta, 0o600); err != nil { - t.Fatalf("setup: %v", err) + return err } + return os.WriteFile(filepath.Join(dir, store.MetaFile), meta, 0o600) } func plantExportDir(t *testing.T, dir string, sub ...string) { diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 629cd8f70..87432c1cb 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -17,9 +17,9 @@ import ( ) const ( - reapDestroy reapAction = iota // engine teardown + drop snapshot - reapArchive // hibernated with archive enabled or on_expire archive: archive, keep the claim - reapPurge // archived past retention: delete the store checkpoint + reapDestroy reapAction = iota + reapArchive + reapPurge reapPause ) @@ -53,7 +53,7 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, o ClaimOptio if err != nil { return nil, err } - // holds belong to this path until finalize; past it the sandbox carries them. + // Holds belong to this path until finalize; past it the sandbox carries them. reserved := applied defer func() { m.unreserveVolumes(reserved) }() m.mu.Lock() @@ -176,7 +176,7 @@ func (m *Manager) Renew(ctx context.Context, id string, cred Cred, ttl time.Dura m.mu.Unlock() return time.Time{}, ErrUnknownSandbox } - // an archived claim's Deadline is a retention window, not a lease: a TTL over it schedules a purge + // An archived claim's Deadline is a retention window, not a lease: a TTL over it schedules a purge if _, archiving := m.archiving[id]; sb.ArchiveCk != "" || archiving { m.mu.Unlock() return time.Time{}, ErrArchived @@ -188,7 +188,7 @@ func (m *Manager) Renew(ctx context.Context, id string, cred Cred, ttl time.Dura if err := m.store.commit(js); err != nil { m.mu.Lock() var rb claimSnapshot - // a concurrent renew that landed keeps its grant: roll back only what this call wrote + // A concurrent renew that landed keeps its grant: roll back only what this call wrote if m.claimed[id] == sb && sb.Deadline.Equal(deadline) { sb.Deadline, sb.LeaseSeconds, sb.OnExpire = prev, prevLease, prevExpire rb = m.store.set(sb) @@ -218,7 +218,7 @@ func (m *Manager) AgentSocket(id, token string) (string, error) { if sb.Replicated() && m.fence.Load().fenced { return "", ErrLeaseLost } - // no activity stamp: a control-plane poll must not keep an idle sandbox awake + // No activity stamp: a control-plane poll must not keep an idle sandbox awake return sb.VsockSocket, nil } @@ -245,12 +245,7 @@ func (m *Manager) dialPort(ctx context.Context, id string, cred Cred, port uint1 sb.Unhold() return nil, err } - release := sb.Unhold - if fenced != nil { - stop := CloseOnFence(fenced, conn) - release = func() { stop(); sb.Unhold() } - } - return &heldConn{Conn: conn, release: sync.OnceFunc(release)}, nil + return newHeldConn(conn, fenced, sb.Unhold), nil } func (m *Manager) dialPassive(ctx context.Context, id string, port uint16) (net.Conn, error) { @@ -283,12 +278,7 @@ func (m *Manager) dialPassive(ctx context.Context, id string, port uint16) (net. sb.UnholdIdle() return nil, err } - release := sb.UnholdIdle - if fenced != nil { - stop := CloseOnFence(fenced, conn) - release = func() { stop(); sb.UnholdIdle() } - } - return &heldConn{Conn: conn, release: sync.OnceFunc(release)}, nil + return newHeldConn(conn, fenced, sb.UnholdIdle), nil } // releaseResolved re-checks under m.mu that sb is still the live claim: no double teardown. @@ -336,7 +326,7 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand logger.Errorf(ctx, saveErr, "persist release of %s", id) return fmt.Errorf("release %s: %w", id, saveErr) } - // cleanup must survive the caller hanging up; the claim is already dropped. + // Cleanup must survive the caller hanging up; the claim is already dropped. ctx = context.WithoutCancel(ctx) if ck != "" { m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM @@ -396,7 +386,7 @@ func (m *Manager) admitClaim(tenant string, volumes []resolvedVolume, applied [] return nil } -// quotaErr answers ErrQuota when the node takes no more claims; callers hold m.mu. +// Callers hold m.mu. func (m *Manager) quotaErr(extra int, tenant string) error { if m.draining { return fmt.Errorf("%w: node draining", ErrQuota) @@ -421,7 +411,7 @@ func (m *Manager) quotaErr(extra int, tenant string) error { return nil } -// tenantDelta tracks tenantLive as m.claimed changes; callers hold m.mu. +// Callers hold m.mu. func (m *Manager) tenantDelta(tenant string, delta int) { if tenant == "" { return @@ -476,7 +466,7 @@ func (m *Manager) finalizeBatch(ctx context.Context, sbs []*types.Sandbox, ttl t if err != nil { m.mu.Unlock() for _, sb := range sbs { - // the rw mount dirtied the journal, so the clean umount is still owed + // The rw mount dirtied the journal, so the clean umount is still owed td := m.quiesceVolumes(ctx, sb) m.removeOrRetry(ctx, sb.VMName, "", td) } @@ -501,7 +491,7 @@ func (m *Manager) finalizeBatch(ctx context.Context, sbs []*types.Sandbox, ttl t return fmt.Errorf("arm egress %s: %w", sb.ID, armErr) } } - // usage lands only after the batch armed, so a rollback leaves no unterminated claim event + // Usage lands only after the batch armed, so a rollback leaves no unterminated claim event for _, sb := range sbs { m.recordUsage(ctx, usage.Event{ Event: "claim", @@ -534,7 +524,6 @@ func (m *Manager) rollbackClaim(ctx context.Context, sbs []*types.Sandbox) { } } -// abortVolumeClaim removes a failed claim's VM and clears reserved against a double release. func (m *Manager) abortVolumeClaim(ctx context.Context, vmName string, reserved *[]types.Volume) { m.removeOrRetry(ctx, vmName, "", volumeTeardown{holds: *reserved}) *reserved = nil @@ -554,7 +543,7 @@ func (m *Manager) reapOnce(ctx context.Context) { var dropped, purges []string var unrows []*types.Sandbox for id, sb := range m.claimed { - // a zero deadline means no expiry (an archived claim kept forever). + // A zero deadline means no expiry (an archived claim kept forever). if sb.Deadline.IsZero() || !now.After(sb.Deadline) { continue } @@ -568,7 +557,7 @@ func (m *Manager) reapOnce(ctx context.Context) { expired = append(expired, victim{action: reapPurge, id: id, ck: sb.ArchiveCk, tenant: sb.Tenant, deadline: sb.Deadline, sb: sb}) purges = append(purges, sb.ArchiveCk) case m.archivesAtDeadline(sb): - // archive instead of destroy, kept in m.claimed for archive() to move + // Archive instead of destroy, kept in m.claimed for archive() to move if _, busy := m.archiving[id]; busy { continue } @@ -653,7 +642,7 @@ func (m *Manager) reapOnce(ctx context.Context) { logger.Error(ctx, saveErr, "persist reap; rolled back") return } - // engine teardown can take minutes, so fan out bounded and never stall the ticker loop + // Engine teardown can take minutes, so fan out bounded and never stall the ticker loop m.runBounded(ctx, len(expired), func(ctx context.Context, i int) { v := expired[i] switch v.action { @@ -778,6 +767,11 @@ type heldConn struct { release func() } +func newHeldConn(conn net.Conn, fenced <-chan struct{}, release func()) *heldConn { + stop := CloseOnFence(fenced, conn) + return &heldConn{Conn: conn, release: sync.OnceFunc(func() { stop(); release() })} +} + func (c *heldConn) Close() error { c.release() return c.Conn.Close() diff --git a/sandboxd/pool/egress_test.go b/sandboxd/pool/egress_test.go index 0b6ec37a2..528eb2090 100644 --- a/sandboxd/pool/egress_test.go +++ b/sandboxd/pool/egress_test.go @@ -17,6 +17,7 @@ import ( "github.com/cocoonstack/sandbox/sandboxd/outbound" "github.com/cocoonstack/sandbox/sandboxd/outbound/outboundtest" "github.com/cocoonstack/sandbox/sandboxd/types" + "github.com/cocoonstack/sandbox/sandboxd/usage" ) var ( @@ -246,7 +247,10 @@ func TestBatchArmFailureRecordsNoUsage(t *testing.T) { if claimed != 0 { t.Errorf("rollback left %d claims", claimed) } - raw, _ := os.ReadFile(filepath.Join(m.dataDir, "usage.jsonl")) + raw, err := os.ReadFile(filepath.Join(m.dataDir, usage.JournalFile)) + if err != nil && !errors.Is(err, os.ErrNotExist) { + t.Fatalf("read usage journal: %v", err) + } if strings.Contains(string(raw), `"ev":"claim"`) { t.Errorf("rolled-back batch left claim usage events:\n%s", raw) } diff --git a/sandboxd/pool/env.go b/sandboxd/pool/env.go index a6f0f2f93..b31ba8f94 100644 --- a/sandboxd/pool/env.go +++ b/sandboxd/pool/env.go @@ -40,7 +40,7 @@ func (m *Manager) writeEnv(ctx context.Context, id, tenant, verb string, next fu if !ok || !tenantOwns(tenant, sb.Tenant) { return ErrUnknownSandbox } - // a guest write must not race a capture + // A guest write must not race a capture locked := sb.Transition.TryLock() if locked { defer sb.Transition.Unlock() diff --git a/sandboxd/pool/failover.go b/sandboxd/pool/failover.go index 6389590a7..ba6f95d12 100644 --- a/sandboxd/pool/failover.go +++ b/sandboxd/pool/failover.go @@ -128,9 +128,6 @@ func (m *Manager) replaceReplica(ctx context.Context, sb *types.Sandbox, ck stri } func (m *Manager) putRow(ctx context.Context, sb *types.Sandbox, ck string, replica, replace bool) (int64, error) { - if m.archives == nil { - return 0, nil - } m.mu.Lock() if replica && m.hasVolumeHolds(sb) { m.mu.Unlock() diff --git a/sandboxd/pool/failover_test.go b/sandboxd/pool/failover_test.go index 505dc55c9..fe4c6a6aa 100644 --- a/sandboxd/pool/failover_test.go +++ b/sandboxd/pool/failover_test.go @@ -1878,6 +1878,7 @@ type failoverCluster struct { reg *archivestest.Mem ckDir string replicate int + volumes []config.VolumeSpec } func newFailoverCluster(t *testing.T) *failoverCluster { @@ -1913,7 +1914,7 @@ func (c *failoverCluster) nodeOn(t *testing.T, eng *fakeEngine, name, dataDir st spec.ReplicateSeconds = c.replicate cfg := &config.Config{ APIToken: "root", DataDir: dataDir, CheckpointDir: c.ckDir, Pools: []config.PoolSpec{spec, {PoolKey: plainKey, Warm: 1}}, - Mesh: &config.MeshConfig{NodeID: name}, + Mesh: &config.MeshConfig{NodeID: name}, Volumes: c.volumes, } b := testBackends(t, cfg) b.Archives = c.reg diff --git a/sandboxd/pool/hibernate.go b/sandboxd/pool/hibernate.go index f983f1765..97507a2f3 100644 --- a/sandboxd/pool/hibernate.go +++ b/sandboxd/pool/hibernate.go @@ -58,7 +58,7 @@ func (m *Manager) agentSocket(ctx context.Context, id, token string, readerOK bo return sock, fenced, sb.Unhold, nil } -// hibernateLocked is Hibernate's body; the caller holds sb.Transition. +// Caller holds sb.Transition. func (m *Manager) hibernateLocked(ctx context.Context, sb *types.Sandbox) error { if sb.ArchiveCk != "" { return nil // already off the node @@ -92,11 +92,11 @@ func (m *Manager) hibernateLocked(ctx context.Context, sb *types.Sandbox) error if err := m.diskErr(); err != nil { return fmt.Errorf("hibernate %s: %w", sb.ID, err) } - // a started transition must finish, or the record disagrees with the VM + // A started transition must finish, or the record disagrees with the VM ctx = context.WithoutCancel(ctx) // cocoon rejects sb.ID's underscore, so the snapshot name is VMName-based snap := hibernatePrefix + strings.TrimPrefix(sb.VMName, vmPrefix) + "-" + randHex(3) - // journal the intent first so Reconcile can adopt a hibernate whose commit never landed + // Journal the intent first so Reconcile can adopt a hibernate whose commit never landed intent, live := m.setIfLive(sb, func() { sb.PendingSnap = snap }) if !live { return ErrUnknownSandbox @@ -117,7 +117,7 @@ func (m *Manager) hibernateLocked(ctx context.Context, sb *types.Sandbox) error sb.ReaderSafe = readerSafe m.mu.Unlock() if hibErr := m.eng.Hibernate(ctx, sb.VMName, snap); hibErr != nil { - // the engine can report failure after the snapshot landed + // The engine can report failure after the snapshot landed adopted, resolveErr := m.settlePendingSnap(ctx, sb) if adopted { m.out.Disarm(sb.ID, true) @@ -137,7 +137,7 @@ func (m *Manager) hibernateLocked(ctx context.Context, sb *types.Sandbox) error m.dropSnap(ctx, snap) return ErrUnknownSandbox } - // the VM is hibernated either way, so the billing window closes here. + // The VM is hibernated either way, so the billing window closes here. m.recordHibernate(ctx, sb) m.out.Disarm(sb.ID, true) if err != nil { @@ -147,7 +147,6 @@ func (m *Manager) hibernateLocked(ctx context.Context, sb *types.Sandbox) error return nil } -// wakeResolved is the wake body shared by the token and id-only entry points. func (m *Manager) wakeResolved(ctx context.Context, sb *types.Sandbox) (string, error) { return m.wakeOrRead(ctx, sb, false) } @@ -168,13 +167,13 @@ func (m *Manager) wakeOrRead(ctx context.Context, sb *types.Sandbox, readerOK bo return "", fmt.Errorf("wake %s: %w", sb.ID, err) } if sb.HibernateSnap == "" { - // no journal sync on this data-plane fast path; restart Reconcile heals the lag + // No journal sync on this data-plane fast path; restart Reconcile heals the lag if sb.StaleSnap != "" && m.store.synced() { m.dropStale(ctx, sb) } return sb.VsockSocket, nil } - // the egress lane never hibernates, so a hibernated one is corrupt state + // The egress lane never hibernates, so a hibernated one is corrupt state if sb.Key.Net == types.NetEgress { return "", fmt.Errorf("wake %s: egress lane cannot resume from hibernation", sb.ID) } @@ -193,7 +192,7 @@ func (m *Manager) wakeOrRead(ctx context.Context, sb *types.Sandbox, readerOK bo snap := sb.HibernateSnap restoredSock, err := m.eng.Restore(ctx, sb.VMName, snap) if err != nil { - // restore may have booted the VM before the engine errored + // Restore may have booted the VM before the engine errored m.stopVM(ctx, sb.VMName) return "", fmt.Errorf("wake %s: %w", sb.ID, err) } @@ -222,7 +221,7 @@ func (m *Manager) wakeOrRead(ctx context.Context, sb *types.Sandbox, readerOK bo return "", ErrUnknownSandbox } if err != nil { - // the lagging journal still references the snapshot, so park it for later reclaim + // The lagging journal still references the snapshot, so park it for later reclaim sb.StaleSnap = snap return "", fmt.Errorf("wake %s: persist claims: %w", sb.ID, err) } @@ -269,7 +268,7 @@ func (m *Manager) idleOnce(ctx context.Context) { m.idleSweep.Store(false) return } - // hibernates are seconds-long engine snapshots, so fan out off the housekeeping loop + // Hibernates are seconds-long engine snapshots, so fan out off the housekeeping loop go func() { defer m.idleSweep.Store(false) m.runBounded(ctx, len(victims), func(ctx context.Context, i int) { @@ -364,7 +363,7 @@ func (m *Manager) resolvePendingSnap(ctx context.Context, sb *types.Sandbox) (ad } if err := m.store.commit(js); err != nil { m.store.recommit(ctx, js) - // the transition is decided in memory, so report the adoption for the caller to bill + // The transition is decided in memory, so report the adoption for the caller to bill return exists, fmt.Errorf("resolve hibernate intent %s: persist: %w", sb.ID, err) } return exists, nil diff --git a/sandboxd/pool/idle_test.go b/sandboxd/pool/idle_test.go index 242704f8c..4c76fea2f 100644 --- a/sandboxd/pool/idle_test.go +++ b/sandboxd/pool/idle_test.go @@ -5,6 +5,7 @@ import ( "net" "net/http" "net/http/httptest" + "sync" "testing" "testing/synctest" "time" @@ -123,6 +124,8 @@ func TestEgressRequestInFlightBlocksIdleSweep(t *testing.T) { _, _ = io.WriteString(w, "ok") })) t.Cleanup(origin.Close) + releaseOrigin := sync.OnceFunc(func() { close(release) }) + t.Cleanup(releaseOrigin) pol := &egress.Policy{Allow: []egress.Rule{{Host: outboundtest.Hostname(t, origin.URL)}}} m := egressManager(t, newFakeEngine(), config.PoolSpec{PoolKey: testKey, IdleHibernateSeconds: 1, Egress: pol}) withOutbound(t, m, func(o *outbound.Options) { o.Dial = (&net.Dialer{}).DialContext }) @@ -149,7 +152,7 @@ func TestEgressRequestInFlightBlocksIdleSweep(t *testing.T) { t.Fatal("claim with an egress request in flight was idle-hibernated") } - close(release) + releaseOrigin() if err := <-result; err != nil { t.Fatalf("request through the proxy: %v", err) } diff --git a/sandboxd/pool/lease.go b/sandboxd/pool/lease.go index 4a136af3a..380d2b624 100644 --- a/sandboxd/pool/lease.go +++ b/sandboxd/pool/lease.go @@ -16,10 +16,6 @@ import ( "github.com/cocoonstack/sandbox/sandboxd/types" ) -type leaseTiming struct { - ttl, every, margin time.Duration -} - // fenceState is the whole fence: while fenced no replicated claim is served, and ch closes when it trips. type fenceState struct { ch chan struct{} @@ -44,27 +40,26 @@ func CloseOnFence(fenced <-chan struct{}, c io.Closer) (stop func()) { // keepLease renews this node's lease until ctx ends and fences its replicated claims when a renewal is overdue. func (m *Manager) keepLease(ctx context.Context) { - fence := time.AfterFunc(m.lease.ttl-m.lease.margin, func() { m.trip(ctx) }) + fence := time.AfterFunc(leaseTTL-leaseMargin, func() { m.trip(ctx) }) defer fence.Stop() - tick := time.NewTicker(m.lease.every) - defer tick.Stop() + tick := time.Tick(leaseRenew) for { m.renewLease(ctx, fence) select { case <-ctx.Done(): return - case <-tick.C: + case <-tick: } } } func (m *Manager) renewLease(ctx context.Context, fence *time.Timer) { start := time.Now() - if err := m.archives.Renew(ctx, m.nodeID, m.lease.ttl); err != nil { + if err := m.archives.Renew(ctx, m.nodeID, leaseTTL); err != nil { log.WithFunc("pool.renewLease").Warnf(ctx, "renew node lease: %v", err) return } - fence.Reset(m.lease.ttl - m.lease.margin - time.Since(start)) + fence.Reset(leaseTTL - leaseMargin - time.Since(start)) if m.isFenced() && m.unfencing.CompareAndSwap(false, true) { go func() { defer m.unfencing.Store(false) @@ -87,7 +82,7 @@ func (m *Manager) trip(ctx context.Context) { continue } close(old.ch) - log.WithFunc("pool.fence").Warnf(ctx, "node lease not renewed for %s; replicated claims answer 503 until it is", m.lease.ttl-m.lease.margin) + log.WithFunc("pool.fence").Warnf(ctx, "node lease not renewed for %s; replicated claims answer 503 until it is", leaseTTL-leaseMargin) return } } @@ -137,7 +132,7 @@ func (m *Manager) lift(ctx context.Context, fenced *fenceState, start time.Time) if !m.fence.CompareAndSwap(fenced, &fenceState{ch: make(chan struct{})}) { return } - if time.Since(start) > m.lease.ttl-m.lease.margin { + if time.Since(start) > leaseTTL-leaseMargin { log.WithFunc("pool.unfence").Warn(ctx, "replicated claims stay fenced: confirming ownership outlasted the lease window") m.trip(ctx) return diff --git a/sandboxd/pool/lease_test.go b/sandboxd/pool/lease_test.go index 36ed91992..022d3b2ef 100644 --- a/sandboxd/pool/lease_test.go +++ b/sandboxd/pool/lease_test.go @@ -212,7 +212,7 @@ func TestLeaseSlowUnfenceStaysFencedAndKeepsRenewing(t *testing.T) { } func TestLeaseFailedConfirmKeepsTheTeardownOfAForgottenClaim(t *testing.T) { - for range 16 { + for range 64 { exercised := false synctest.Test(t, func(t *testing.T) { c := newReplicaCluster(t) diff --git a/sandboxd/pool/liveness.go b/sandboxd/pool/liveness.go index 640746831..9ab5164a1 100644 --- a/sandboxd/pool/liveness.go +++ b/sandboxd/pool/liveness.go @@ -18,7 +18,7 @@ import ( const ( vmmRestartBudget = 3 vmmRestartWindow = 10 * time.Minute - // with the cocoon daemon stream live, the list only catches an event the stream dropped + // With the cocoon daemon stream live, the list only catches an event the stream dropped livenessBackstop = time.Minute vmEventsBackoff = time.Second vmEventsMaxBackoff = 30 * time.Second @@ -139,7 +139,7 @@ func (m *Manager) recoverVMM(ctx context.Context, sb *types.Sandbox) { return } logger := log.WithFunc("pool.recoverVMM") - // the sweep's list predates this lock, so a wake that finished in between reads as down there + // The sweep's list predates this lock, so a wake that finished in between reads as down there vm, found, err := m.eng.Inspect(ctx, sb.VMName) switch { case err != nil: @@ -219,7 +219,7 @@ func (m *Manager) restartLocked(ctx context.Context, sb *types.Sandbox) error { m.mu.Lock() sb.VsockSocket = sock m.mu.Unlock() - // a displaced door's close unlinks the rebound path, so the old door goes first + // A displaced door's close unlinks the rebound path, so the old door goes first m.out.Disarm(sb.ID, false) err = m.out.ArmProxy(ctx, sb) if err == nil { @@ -235,7 +235,7 @@ func (m *Manager) restartLocked(ctx context.Context, sb *types.Sandbox) error { sb.Restarts++ sb.RestartedAt = time.Now() }); !live { - // the release that dropped the claim owns the VM teardown + // The release that dropped the claim owns the VM teardown m.out.Disarm(sb.ID, true) return ErrUnknownSandbox } @@ -274,7 +274,7 @@ func coldBootBlocker(sb *types.Sandbox, found, volumes bool) string { case !found: return "vm record gone" case sb.Key.Net == types.NetEgress: - // the netdev nft chain dies with the tap, so the guest would boot with an unlocked NIC + // The netdev nft chain dies with the tap, so the guest would boot with an unlocked NIC return "vmm exited; an egress-lane sandbox cannot cold-boot" case volumes: return "vmm exited; volume mounts do not survive a cold boot" diff --git a/sandboxd/pool/liveness_test.go b/sandboxd/pool/liveness_test.go index f1a895868..83b6b3762 100644 --- a/sandboxd/pool/liveness_test.go +++ b/sandboxd/pool/liveness_test.go @@ -294,6 +294,9 @@ func TestRestartThatCannotDeliverTheEnvFailsAndSpendsTheBudget(t *testing.T) { } func TestRestartThatCannotArmTheEgressDoorFailsAndSpendsTheBudget(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores file permissions") + } eng := newFakeEngine() eng.sockRoot = outboundtest.SockRoot(t) m := egressManager(t, eng, config.PoolSpec{PoolKey: testKey, Egress: &egress.Policy{Allow: []egress.Rule{{Host: "example.com"}}}}) diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 8bcd808c3..50d4d018b 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -367,7 +367,6 @@ type Manager struct { orphanSweep atomic.Bool adoptFlights singleflight.Group // fence gates replicated claims while the lease is overdue; replicaPools is fixed at start; replicas belongs to the pass holding replicating. - lease leaseTiming fence atomic.Pointer[fenceState] unfencing atomic.Bool replicaPools map[types.PoolKey]struct{} @@ -458,7 +457,6 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg poolStore: newPoolStore(cfg.DataDir), poolShared: b.CellPools, archives: b.Archives, - lease: leaseTiming{ttl: leaseTTL, every: leaseRenew, margin: leaseMargin}, tenants: b.Tenants, ckpts: b.Checkpoints, ckptsShared: b.SharedCheckpoints, diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index df7a1ce4a..6bc3c3910 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -998,24 +998,21 @@ func TestRenewRefusesAClaimBeingArchived(t *testing.T) { } func TestRenewRollsBackOnPersistFailure(t *testing.T) { - dir := t.TempDir() - m := newTestManagerAt(t, newFakeEngine(), dir) + m := newTestManager(t, newFakeEngine()) sb := mustClaim(t, m, testKey) before, beforeLease := sb.Deadline, sb.LeaseSeconds + broken := filepath.Join(t.TempDir(), "gone", "claims.json") + m.store.path = broken - if err := os.Chmod(dir, 0o500); err != nil { - t.Fatalf("chmod: %v", err) - } - _, err := m.Renew(t.Context(), sb.ID, Cred{Token: sb.Token}, time.Hour, "") - if chmodErr := os.Chmod(dir, 0o700); chmodErr != nil { - t.Fatalf("restore mode: %v", chmodErr) - } - if err == nil { + if _, err := m.Renew(t.Context(), sb.ID, Cred{Token: sb.Token}, time.Hour, ""); err == nil { t.Fatal("renew reported success while the claim store was unwritable") } if !sb.Deadline.Equal(before) || sb.LeaseSeconds != beforeLease { t.Errorf("deadline %v lease %ds after a failed persist, want %v / %ds", sb.Deadline, sb.LeaseSeconds, before, beforeLease) } + if err := os.MkdirAll(filepath.Dir(broken), 0o750); err != nil { + t.Fatalf("heal store dir: %v", err) + } granted, err := m.Renew(t.Context(), sb.ID, Cred{Token: sb.Token}, time.Hour, "") if err != nil { t.Fatalf("renew after a rolled-back one: %v", err) diff --git a/sandboxd/pool/promote_test.go b/sandboxd/pool/promote_test.go index ba6c5971e..0da536f93 100644 --- a/sandboxd/pool/promote_test.go +++ b/sandboxd/pool/promote_test.go @@ -139,6 +139,43 @@ func TestPromoteValidations(t *testing.T) { } } +func TestTemplateDigestGuardReadsTheSharedRecord(t *testing.T) { + eng := newFakeEngine() + eng.exportContent = []byte("generation one") + m := newTestManager(t, eng) + parent := mustClaim(t, m, testKey) + key, stale, err := m.Promote(t.Context(), parent.ID, Cred{Token: parent.Token}, "tpl:shared", "") + if err != nil { + t.Fatalf("Promote: %v", err) + } + peer := newTestManager(t, newFakeEngine()) + peer.tpls = m.tpls + id := store.TemplateID(key.Hash()) + peer.tplSet[id] = TemplateInfo{Key: key, ContentDigest: stale} + + eng.exportContent = []byte("generation two") + _, latest, err := m.Promote(t.Context(), parent.ID, Cred{Token: parent.Token}, "tpl:shared", "") + if err != nil { + t.Fatalf("re-Promote: %v", err) + } + if err := peer.SetTemplateLabels(t.Context(), key, types.Metadata{"a": "b"}, "", stale); !errors.Is(err, ErrTemplateReplaced) { + t.Errorf("labels with the replaced digest: %v, want ErrTemplateReplaced", err) + } + if err := peer.DeleteTemplate(t.Context(), key, "", stale); !errors.Is(err, ErrTemplateReplaced) { + t.Errorf("delete with the replaced digest: %v, want ErrTemplateReplaced", err) + } + if _, err := m.tpls.ReadMeta(t.Context(), id); err != nil { + t.Fatalf("the newer build is gone: %v", err) + } + delete(peer.tplSet, id) + if err := peer.SetTemplateLabels(t.Context(), key, types.Metadata{"a": "b"}, "", latest); err != nil { + t.Errorf("labels with the current digest on a node that has not cached it: %v", err) + } + if err := peer.DeleteTemplate(t.Context(), key, "", latest); err != nil { + t.Errorf("delete with the current digest on a node that has not cached it: %v", err) + } +} + func TestDeleteTemplate(t *testing.T) { eng := newFakeEngine() m := newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 0}) diff --git a/sandboxd/pool/reader.go b/sandboxd/pool/reader.go index 8e598028b..85dd05e35 100644 --- a/sandboxd/pool/reader.go +++ b/sandboxd/pool/reader.go @@ -49,8 +49,7 @@ func (m *Manager) DialReader(ctx context.Context, id, token string) (conn net.Co m.dropReader(ctx, r, true) return nil, "", fmt.Errorf("%w: %w", ErrNoReader, err) } - stop := CloseOnFence(fenced, conn) - return &heldConn{Conn: conn, release: sync.OnceFunc(func() { stop(); m.releaseReader(r) })}, r.source, nil + return newHeldConn(conn, fenced, func() { m.releaseReader(r) }), r.source, nil } // SleepingSource names the hibernate snapshot or archive record a reader would serve, or "" when the claim is awake, failed or mid-transition. diff --git a/sandboxd/pool/reader_test.go b/sandboxd/pool/reader_test.go index 9a4e1067b..7f62a4925 100644 --- a/sandboxd/pool/reader_test.go +++ b/sandboxd/pool/reader_test.go @@ -603,9 +603,11 @@ func TestWakeAgentSocketStampsActivityOnlyWhenItServesTheGuest(t *testing.T) { for _, on := range []bool{false, true} { t.Run(fmt.Sprintf("reader=%t", on), func(t *testing.T) { eng := newFakeEngine() - m := newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 1}) + var m *Manager if on { m = newReaderManager(t, eng) + } else { + m = newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 1}) } sb := mustClaim(t, m, testKey) backdate(m, sb, time.Hour) @@ -765,8 +767,8 @@ func TestHibernateSyncsTheGuestOnlyWithTheReaderOn(t *testing.T) { eng.hibernateStall = make(chan struct{}) hibernated := make(chan struct{}) go func() { + defer close(hibernated) hibernateClaim(t, m, sb) - close(hibernated) }() waitFor(t, func() bool { eng.mu.Lock(); defer eng.mu.Unlock(); return len(eng.hibernates) == 1 }) eng.mu.Lock() diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index b53f9bb34..ec69b2a34 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -46,25 +46,25 @@ func (m *Manager) Reconcile(ctx context.Context) error { rec, ok := live[sb.VMName] switch { case sb.ArchiveCk != "": - // archived: no local VM by design, so adopt the stub and let the first exec wake it + // Archived: no local VM by design, so adopt the stub and let the first exec wake it m.addClaimLocked(sb) continue case ok && rec.State == vmStateRunning: sb.VsockSocket = rec.VsockSocket - // running with either flag set means a crashed wake or an intent the engine never saw + // Running with either flag set means a crashed wake or an intent the engine never saw if sb.HibernateSnap != "" { resumed = append(resumed, sb) } sb.HibernateSnap, sb.PendingSnap = "", "" case ok && sb.HibernateSnap != "": - // hibernated: the VM is stopped by design and wakes on demand. + // Hibernated: the VM is stopped by design and wakes on demand. sb.PendingSnap = "" case ok && sb.PendingSnap != "" && (snapsErr != nil || slices.Contains(snaps, sb.PendingSnap)): - // the journaled intent names the wake image, so adopt it when the snapshot is there + // The journaled intent names the wake image, so adopt it when the snapshot is there sb.HibernateSnap, sb.PendingSnap, sb.ReaderSafe = sb.PendingSnap, "", false adopted = append(adopted, sb) case ok: - // the VMM died with the host or on its own; the liveness pass restarts or fails it + // The VMM died with the host or on its own; the liveness pass restarts or fails it sb.PendingSnap = "" default: dropped = append(dropped, sb) @@ -107,7 +107,7 @@ func (m *Manager) Reconcile(ctx context.Context) error { logger.Infof(ctx, "removed orphan golden %s", name) } } - // a crash mid-export leaves a fork-* staging dir nothing in this life reuses + // A crash mid-export leaves a fork-* staging dir nothing in this life reuses _ = utils.RemoveDirEntries(m.dataDir, func(name string) bool { return strings.HasPrefix(name, "fork-") }) _ = utils.RemoveDirEntries(filepath.Join(m.dataDir, readerDir), nil) @@ -117,7 +117,7 @@ func (m *Manager) Reconcile(ctx context.Context) error { m.retryArchiveDeletes(ctx) removed := m.sweepStaleVMs(ctx, live, owned) - // an unreferenced hibernate snapshot is an orphan; fork and golden snapshots never span a restart + // An unreferenced hibernate snapshot is an orphan; fork and golden snapshots never span a restart if snapsErr != nil { logger.Warnf(ctx, "snapshot sweep skipped: %v", snapsErr) } else { @@ -166,10 +166,10 @@ func (m *Manager) sweepStaleVMs(ctx context.Context, live map[string]types.VMRec func (m *Manager) removeStaleVM(ctx context.Context, name string, rec types.VMRecord) bool { logger := log.WithFunc("pool.removeStaleVM") if rec.State == vmStateCreating { - // a canceled ctx must not skip the busy check guarding the forced remove + // A canceled ctx must not skip the busy check guarding the forced remove switch outcome, err := m.eng.ReconcileStaleCreate(context.WithoutCancel(ctx), name); { case err != nil: - // verb missing (cocoon < v0.5.8) or failed: keep the old sweep. + // Verb missing (cocoon < v0.5.8) or failed: keep the old sweep. logger.Warnf(ctx, "reconcile stale create %s: %v; removing", name, err) case outcome == engine.StaleCreateCollected, outcome == engine.StaleCreateNotFound: return true @@ -178,7 +178,7 @@ func (m *Manager) removeStaleVM(ctx context.Context, name string, rec types.VMRe m.queueStaleCreate(name, rec.TapDevice()) return false } - // not-creating: the record moved on under the lock; remove normally. + // Not-creating: the record moved on under the lock; remove normally. } if m.removeVM(ctx, name) { return true @@ -217,13 +217,13 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco var quarantine []*types.Sandbox for _, sb := range m.claimed { sb.TouchAt(now) - // a hibernated or archived claim has no guest to serve; its wake arms the door + // A hibernated or archived claim has no guest to serve; its wake arms the door if sb.HibernateSnap != "" || sb.ArchiveCk != "" || sb.Failed != "" { continue } if m.out.LocksNIC(sb.Key) { tap := m.readoptEgressTap(sb, live) - // a down egress-lane VM has no guest to lock or mark; the liveness pass fails it + // A down egress-lane VM has no guest to lock or mark; the liveness pass fails it if live[sb.VMName].State != vmStateRunning { continue } @@ -246,7 +246,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco logger.Errorf(ctx, proxyErr, "arm egress proxy %s", sb.ID) } } - // quarantine before the sweep so a failed remove's still-running tap is kept. + // Quarantine before the sweep so a failed remove's still-running tap is kept. keep := make(map[string]bool, len(live)) for _, sb := range quarantine { if m.quarantineClaim(ctx, sb) { @@ -255,7 +255,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco keep[sb.TAP] = true } } - // keep every still-running VM's tap; sweep only a confirmed-gone VM's table. + // Keep every still-running VM's tap; sweep only a confirmed-gone VM's table. for name, rec := range live { if tap := rec.TapDevice(); tap != "" && !removed[name] { keep[tap] = true diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index 37c7d4b58..a2d08be55 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -35,7 +35,7 @@ func (m *Manager) refillOnce(ctx context.Context) { return } now := time.Now() - // spawning VMs into a full node still takes the global rtnl lock even when doomed to fail + // Spawning VMs into a full node still takes the global rtnl lock even when doomed to fail parked := m.capacityReasonLocked(now) != "" inFlight := 0 for _, p := range m.pools { @@ -121,7 +121,7 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string, gen uin p.noteLead(time.Since(start)) keep = true } - // a canceled context or a capacity failure says nothing about this pool's health + // A canceled context or a capacity failure says nothing about this pool's health backedOff := false if ctx.Err() == nil && capReason == "" { backedOff = p.noteRefillResult(now, err != nil) @@ -210,7 +210,7 @@ func (m *Manager) buildGolden(ctx context.Context, p *pool) { p.goldenGen++ default: p.nextBuild = time.Now().Add(buildRetryDelay) - // a cold boot hits the same bridge and disk as a refill, so it feeds the park too + // A cold boot hits the same bridge and disk as a refill, so it feeds the park too if reason := engine.CapacitySignature(err); reason != "" { m.noteCapacityLocked(time.Now(), reason) } @@ -266,7 +266,7 @@ func (m *Manager) runWarmup(ctx context.Context, warmup []string, sock string) e return m.eng.Warmup(ctx, sock, warmup) } -// a restore maps the golden memory lazily, so the warmup's pages fault in here instead of under the first claim +// A restore maps the golden memory lazily, so the warmup's pages fault in here instead of under the first claim func (m *Manager) warmClone(ctx context.Context, sb *types.Sandbox) error { if err := m.runWarmup(ctx, m.view.Load().poolWarmups[sb.Key], sb.VsockSocket); err != nil { m.destroy(ctx, sb.VMName) @@ -310,7 +310,7 @@ func (m *Manager) goldenStamp(v *configView, key types.PoolKey, imageID string) fp = m.out.CAFingerprint() } fields := []string{fp, string(m.out.Lane(key)), imageID} - // only a sized pool stamps its disk, in bytes so "40G" and "40GiB" match and unsized goldens stay adoptable + // Only a sized pool stamps its disk, in bytes so "40G" and "40GiB" match and unsized goldens stay adoptable if storage := v.poolStorage[key]; storage != "" { n, _ := config.StorageBytes(storage) fields = append(fields, "storage="+strconv.FormatInt(n, 10)) @@ -368,7 +368,6 @@ func (m *Manager) exportGolden(ctx context.Context, snap, final string) error { return os.Rename(tmp, final) } -// provision creates one claim-ready VM, cloning from a golden when available. func (m *Manager) provision(ctx context.Context, key types.PoolKey, golden string) (*types.Sandbox, error) { if golden == "" { return m.provisionCold(ctx, key) @@ -385,7 +384,7 @@ func (m *Manager) provisionCold(ctx context.Context, key types.PoolKey) (*types. if err != nil { return nil, err } - // a pre-golden cold claim must trust the root, or intercepted hosts fail TLS for its life + // A pre-golden cold claim must trust the root, or intercepted hosts fail TLS for its life if err := m.out.PrepareGuest(ctx, m.view.Load().out, key, sb.VsockSocket); err != nil { m.destroy(ctx, sb.VMName) return nil, err @@ -503,7 +502,7 @@ func (m *Manager) destroyAll(ctx context.Context, names []string) *sync.WaitGrou func (m *Manager) runBounded(ctx context.Context, n int, f func(context.Context, int)) *sync.WaitGroup { var wg sync.WaitGroup for i := range n { - // acquire inside the goroutine so a batch larger than the budget cannot block the caller + // Acquire inside the goroutine so a batch larger than the budget cannot block the caller wg.Go(func() { for _, sem := range [...]chan struct{}{m.maintSem, m.refillSem} { select { diff --git a/sandboxd/pool/refillkick_test.go b/sandboxd/pool/refillkick_test.go index 256149840..7dc7552d8 100644 --- a/sandboxd/pool/refillkick_test.go +++ b/sandboxd/pool/refillkick_test.go @@ -21,7 +21,7 @@ func TestKickRefillCoalescesAndNeverBlocks(t *testing.T) { <-m.refillKick m.kickRefill() if len(m.refillKick) != 1 { - t.Fatalf("kick after drain not delivered") + t.Fatal("kick after drain not delivered") } } diff --git a/sandboxd/pool/reload_test.go b/sandboxd/pool/reload_test.go index 24cbdbc24..50e1ad45b 100644 --- a/sandboxd/pool/reload_test.go +++ b/sandboxd/pool/reload_test.go @@ -13,6 +13,7 @@ import ( "reflect" "slices" "strings" + "sync" "sync/atomic" "testing" @@ -229,6 +230,8 @@ func TestAReloadDropsTheLiveProxiesPooledConnections(t *testing.T) { } origin.Start() t.Cleanup(origin.Close) + releaseOrigin := sync.OnceFunc(func() { close(release) }) + t.Cleanup(releaseOrigin) host := outboundtest.Hostname(t, origin.URL) m := egressManager(t, newFakeEngine(), config.PoolSpec{PoolKey: testKey, Warm: 1, Egress: &egress.Policy{Allow: []egress.Rule{{Host: host}}}}) withOutbound(t, m, func(o *outbound.Options) { o.Dial = (&net.Dialer{}).DialContext }) @@ -256,7 +259,7 @@ func TestAReloadDropsTheLiveProxiesPooledConnections(t *testing.T) { if err := get("/"); err != nil { t.Fatalf("get after the reload: %v", err) } - close(release) + releaseOrigin() if err := <-inflight; err != nil { t.Fatalf("in-flight get: %v", err) } diff --git a/sandboxd/pool/remove.go b/sandboxd/pool/remove.go index 75efec26e..ebea44ca1 100644 --- a/sandboxd/pool/remove.go +++ b/sandboxd/pool/remove.go @@ -15,7 +15,7 @@ import ( func (m *Manager) removeVM(ctx context.Context, name string) bool { m.out.ClosePrebound(name) - // cancellation-immune: on a canceled ctx `cocoon vm rm` no-ops and orphans. + // Cancellation-immune: on a canceled ctx `cocoon vm rm` no-ops and orphans. ctx = context.WithoutCancel(ctx) err := m.eng.Remove(ctx, name) if err == nil { @@ -31,7 +31,7 @@ func (m *Manager) confirmGone(ctx context.Context, name string) bool { logger := log.WithFunc("pool.confirmGone") _, present, err := m.eng.Inspect(ctx, name) if err != nil { - // cannot tell: a false "gone" leaks a running VM, a retry only costs a sweep + // Cannot tell: a false "gone" leaks a running VM, a retry only costs a sweep logger.Warnf(ctx, "verify remove of %s: %v", name, err) return false } diff --git a/sandboxd/pool/setpools.go b/sandboxd/pool/setpools.go index d4d74357c..f22558d64 100644 --- a/sandboxd/pool/setpools.go +++ b/sandboxd/pool/setpools.go @@ -40,7 +40,7 @@ func (m *Manager) desiredPools(specs []config.PoolSpec) (map[types.PoolKey]confi if err := spec.ValidateLimits(); err != nil { return nil, fmt.Errorf("%w: %w", ErrBadCount, err) } - // config-owned fields; accepting one here would silently drop it + // Config-owned fields; accepting one here would silently drop it for _, f := range []struct { name string set bool @@ -74,12 +74,12 @@ func (m *Manager) applyPools(ctx context.Context, desired map[types.PoolKey]conf return nil } m.poolVersion = version - // sequenced under the mutex (apply order) so commit drops a write a later apply superseded. + // Sequenced under the mutex (apply order) so commit drops a write a later apply superseded. seq := m.poolStore.seq.Add(1) now := time.Now() for key, p := range m.pools { spec, ok := desired[key] - // a removed key gets the zero spec, so a lingering pool sheds a stale archiveAfter + // A removed key gets the zero spec, so a lingering pool sheds a stale archiveAfter p.applySpec(spec) p.removed = !ok trim = append(trim, p.trimWarm(p.effectiveTarget(now))...) @@ -100,7 +100,7 @@ func (m *Manager) applyPools(ctx context.Context, desired map[types.PoolKey]conf runCtx := context.WithoutCancel(ctx) m.destroyAll(runCtx, trim).Wait() m.refillOnce(runCtx) - // persist the applied set so a restart rebuilds from it, not the config seed. + // Persist the applied set so a restart rebuilds from it, not the config seed. return m.poolStore.commit(seq, poolset.Set{ConfigSeed: m.configSeedHash, Pools: slices.Collect(maps.Values(desired))}) } diff --git a/sandboxd/pool/stats.go b/sandboxd/pool/stats.go index 0e11db4ff..20f781a1e 100644 --- a/sandboxd/pool/stats.go +++ b/sandboxd/pool/stats.go @@ -22,7 +22,7 @@ type SandboxStats struct { // Stats reports one live claim's resource usage. func (m *Manager) Stats(ctx context.Context, id string) (SandboxStats, bool) { - // hibernate and archive rewrite HibernateSnap and VMName under m.mu; the /proc read needs none + // Hibernate and archive rewrite HibernateSnap and VMName under m.mu; the /proc read needs none m.mu.Lock() sb := m.claimed[id] if sb == nil { diff --git a/sandboxd/pool/stats_test.go b/sandboxd/pool/stats_test.go index 3235a95c8..ddfb142bc 100644 --- a/sandboxd/pool/stats_test.go +++ b/sandboxd/pool/stats_test.go @@ -26,8 +26,14 @@ func TestOperatorReadsRaceFreeUnderHibernate(t *testing.T) { wg.Go(func() { defer close(done) for range 100 { - _ = m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}) - _ = m.Wake(t.Context(), sb.ID, Cred{Token: sb.Token}) + if err := m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Errorf("hibernate: %v", err) + return + } + if err := m.Wake(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Errorf("wake: %v", err) + return + } } }) diff --git a/sandboxd/pool/telemetry.go b/sandboxd/pool/telemetry.go index 97c1ff1c6..53e83c381 100644 --- a/sandboxd/pool/telemetry.go +++ b/sandboxd/pool/telemetry.go @@ -158,7 +158,7 @@ func (m *Manager) TenantClaims() map[string]int { // Audit records one relayed request frame against a sandbox: the op and addressing fields only. func (m *Manager) Audit(ctx context.Context, id string, line []byte) { if len(line) > AuditLineCap { - // the addressing fields stay unread at this size, but a padded frame must not escape the journal + // The addressing fields stay unread at this size, but a padded frame must not escape the journal m.recordAudit(ctx, id, auditFrame{Op: "oversized"}) return } @@ -194,7 +194,6 @@ func (m *Manager) appendAudit(ctx context.Context, event any) { } } -// recordEgress logs one guarded-egress decision and meters it as a usage event. func (m *Manager) recordEgress(ctx context.Context, id, tenant string, ev egress.Event) { decision := "deny" switch ev.Decision { @@ -217,7 +216,6 @@ func (m *Manager) recordEgress(ctx context.Context, id, tenant string, ev egress m.recordUsage(ctx, usage.Event{Event: egressEvent, ID: id, Tenant: tenant, Reference: ev.Host, Upstream: ev.Upstream, Secret: ev.Injected}) } -// recordUsage appends one billing event; failures are logged, never propagated. func (m *Manager) recordUsage(ctx context.Context, ev usage.Event) { ev.Time = time.Now() if err := m.usage.append(ev); err != nil { diff --git a/sandboxd/pool/template.go b/sandboxd/pool/template.go index 528bf4d06..e3307e2df 100644 --- a/sandboxd/pool/template.go +++ b/sandboxd/pool/template.go @@ -56,14 +56,14 @@ func (m *Manager) Promote(ctx context.Context, id string, cred Cred, template, t } key := types.PoolKey{Template: template, Net: sb.Key.Net, Size: sb.Key.Size} if m.pooled(key) { - // a configured pool owns this key; promoting over it would change what refills produce + // A configured pool owns this key; promoting over it would change what refills produce return types.PoolKey{}, "", ErrPooledTemplate } // commitTemplate re-checks under the template lock; this only fast-fails before the export if err := m.checkTemplateOwner(ctx, store.TemplateID(key.Hash()), tenant); err != nil { return types.PoolKey{}, "", err } - // a started promote must finish uncanceled + // A started promote must finish uncanceled ctx = context.WithoutCancel(ctx) staging, err := m.tpls.Stage(store.TemplateID(key.Hash())) if err != nil { @@ -103,11 +103,8 @@ func (m *Manager) DeleteTemplate(ctx context.Context, key types.PoolKey, tenant, if err := m.ownedTemplate(ctx, id, tenant); err != nil { return err } - m.tplMu.Lock() - held := m.tplSet[id].ContentDigest - m.tplMu.Unlock() - if digest != "" && held != digest { - return ErrTemplateReplaced + if err := m.checkTemplateDigest(ctx, id, digest); err != nil { + return err } if err := m.tpls.Delete(ctx, id); err != nil { return fmt.Errorf("delete template: %w", err) @@ -149,11 +146,8 @@ func (m *Manager) SetTemplateLabels(ctx context.Context, key types.PoolKey, labe if err != nil { return err } - m.tplMu.Lock() - held := m.tplSet[id].ContentDigest - m.tplMu.Unlock() - if digest != "" && held != digest { - return ErrTemplateReplaced + if err = m.checkTemplateDigest(ctx, id, digest); err != nil { + return err } var payload []byte if len(labels) == 0 { @@ -208,7 +202,7 @@ func (m *Manager) HasPromotedTemplate(ctx context.Context, key types.PoolKey, te m.tplMu.Unlock() owner := held.Tenant if !cached { - // only a shared-store template promoted elsewhere after startup. + // Only a shared-store template promoted elsewhere after startup. raw, err := m.tpls.ReadMeta(ctx, id) if err != nil { return false @@ -240,6 +234,20 @@ func (m *Manager) ownedTemplate(ctx context.Context, id, tenant string) error { return nil } +func (m *Manager) checkTemplateDigest(ctx context.Context, id, digest string) error { + if digest == "" { + return nil + } + held, err := m.tpls.ReadDigest(ctx, id) + if err != nil { + return fmt.Errorf("read template digest: %w", err) + } + if held != digest { + return ErrTemplateReplaced + } + return nil +} + // unpooledTemplates copies the held templates by id, less any key a pool now owns. func (m *Manager) unpooledTemplates() map[string]TemplateInfo { held := map[string]TemplateInfo{} diff --git a/sandboxd/pool/tenants_test.go b/sandboxd/pool/tenants_test.go index 36b315eca..403af0dd3 100644 --- a/sandboxd/pool/tenants_test.go +++ b/sandboxd/pool/tenants_test.go @@ -95,7 +95,7 @@ func TestRemovedTenantRunsToItsDeadlineButCannotExtend(t *testing.T) { if got := m.Sandboxes("", "", nil); len(got) != 1 || got[0].Tenant != "acme" { t.Fatalf("root listing %+v, want the claim kept under acme", got) } - infos, _ := listTenants(t, m) + infos := listTenants(t, m) if !slices.Contains(infos, TenantInfo{Name: "acme", Claims: 1, Removed: true}) { t.Errorf("Tenants %+v, want acme listed as removed with its claim", infos) } @@ -345,7 +345,7 @@ func TestARuntimeTenantTakesItsEgressClassWithoutARestart(t *testing.T) { if classed.EgressClass != "desk" || later.EgressClass != "" { t.Errorf("classes live %q new %q: a class change reaches only claims made after it", classed.EgressClass, later.EgressClass) } - infos, _ := listTenants(t, m) + infos := listTenants(t, m) if i := slices.IndexFunc(infos, func(ti TenantInfo) bool { return ti.Name == "u-1" }); i < 0 || infos[i].EgressClass != "" { t.Errorf("tenant list %+v, want u-1 without a class", infos) } @@ -417,13 +417,13 @@ func tenantOf(t *testing.T, m *Manager, token string) (string, bool) { return r.Name, true } -func listTenants(t *testing.T, m *Manager) ([]TenantInfo, string) { +func listTenants(t *testing.T, m *Manager) []TenantInfo { t.Helper() page, err := m.Tenants(t.Context(), "", 1000) if err != nil { t.Fatalf("list tenants: %v", err) } - return page.Tenants, page.Digest + return page.Tenants } func tenantManager(t *testing.T, dir string, tenants ...tenants.TenantSpec) *Manager { diff --git a/sandboxd/pool/upstream_test.go b/sandboxd/pool/upstream_test.go index c39daf4e4..a42946e66 100644 --- a/sandboxd/pool/upstream_test.go +++ b/sandboxd/pool/upstream_test.go @@ -64,6 +64,9 @@ func TestPatchSwitchesTheUpstreamForNewConnections(t *testing.T) { if got := upstream(); got != "http://127.0.0.1:3001" { t.Fatalf("upstream %q after the first patch", got) } + if got := sb.Env.Redacted()["EGRESS_UPSTREAM"].Value; got != "" { + t.Errorf("GET env would show %q", got) + } if err := m.PatchEnv(t.Context(), sb.ID, types.EnvPatch{"EGRESS_UPSTREAM": nil}, ""); err != nil { t.Fatalf("clear: %v", err) } @@ -73,9 +76,6 @@ func TestPatchSwitchesTheUpstreamForNewConnections(t *testing.T) { if err := m.PatchEnv(t.Context(), sb.ID, types.EnvPatch{"EGRESS_UPSTREAM": {Value: "http://10.9.9.9:1", Guest: hostOnly}}, ""); !errors.Is(err, ErrBadEnv) { t.Errorf("patch to an unlisted upstream: %v, want ErrBadEnv", err) } - if got := sb.Env.Redacted()["EGRESS_UPSTREAM"].Value; got != "" { - t.Errorf("GET env would show %q", got) - } } func upstreamManager(t *testing.T, allow ...string) *Manager { diff --git a/sandboxd/pool/volume.go b/sandboxd/pool/volume.go index aee84b925..8133fbf9e 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -19,7 +19,7 @@ import ( ) const ( - // sidecar, not data_dir: the marker travels with the image and survives a data_dir wipe + // Sidecar, not data_dir: the marker travels with the image and survives a data_dir wipe volumeDirtySuffix = ".dirty" volumeQuiesceMax = 10 * time.Second ) @@ -74,7 +74,7 @@ func (m *Manager) Volumes(tenant string, holders map[string]int) []types.VolumeI } infos = append(infos, info) } - // root can see peer-only names; a tenant needs local catalog metadata to apply the access list + // Root can see peer-only names; a tenant needs local catalog metadata to apply the access list if tenant == "" { for name, nodes := range holders { if _, ok := catalog[name]; !ok { @@ -164,7 +164,7 @@ func (m *Manager) resolveCatalogEntry(ctx context.Context, volume types.Volume, return resolvedVolume{}, fmt.Errorf("%w: volume %q is not writable", ErrBadVolume, volume.Name) } if _, statErr := os.Stat(entry.disk.Path); statErr != nil { - // the operator's path stays server-side; the refusal matches the unknown one + // The operator's path stays server-side; the refusal matches the unknown one log.WithFunc("pool.resolveCatalogEntry").Warnf(ctx, "stat volume %q image %s: %v", volume.Name, entry.disk.Path, statErr) return resolvedVolume{}, ErrVolumeUnavailable } @@ -201,9 +201,9 @@ func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes [ // applyVolume keeps one volume's steps strictly ordered; siblings overlap freely. func (m *Manager) applyVolume(ctx context.Context, sb *types.Sandbox, volume resolvedVolume, live bool) error { - // attach-only mounts nothing, so there is no umount to verify and no marker. + // Attach-only mounts nothing, so there is no umount to verify and no marker. attachOnly := volume.applied.AttachOnly - // write-ahead: the marker must be durable before any guest write can be. + // Write-ahead: the marker must be durable before any guest write can be. if volume.disk.RW && !attachOnly { if err := markVolumeDirty(volume.disk.Path); err != nil { return fmt.Errorf("mark volume %q dirty: %w", volume.applied.Name, err) @@ -237,7 +237,7 @@ func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeT } catalog := m.catalogSnapshot() logger := log.WithFunc("pool.quiesceVolumes") - // cancellation-immune like removal: a caller hanging up must not skip the flush. + // Cancellation-immune like removal: a caller hanging up must not skip the flush. ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), quiesceBudget(mounts)) defer cancel() stuck := false @@ -256,7 +256,7 @@ func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeT } } if stuck { - // the unmounts may have spent the whole budget; the fallback flush gets its own + // The unmounts may have spent the whole budget; the fallback flush gets its own syncCtx, cancelSync := context.WithTimeout(context.WithoutCancel(ctx), engine.VolumeCallTimeout) defer cancelSync() if err := m.eng.SyncGuest(syncCtx, sb.VsockSocket); err != nil { diff --git a/sandboxd/pool/volume_replica_test.go b/sandboxd/pool/volume_replica_test.go index 29746fe5c..065ae4d1b 100644 --- a/sandboxd/pool/volume_replica_test.go +++ b/sandboxd/pool/volume_replica_test.go @@ -104,22 +104,8 @@ func TestReplicaCaptureRefusesPendingLiveWriter(t *testing.T) { func newReplicaVolumeManager(t *testing.T) (*Manager, *failoverCluster) { t.Helper() c := newReplicaCluster(t) - spec := archivePool(keepAnHour) - spec.ReplicateSeconds = c.replicate - cfg := &config.Config{ - APIToken: "root", DataDir: t.TempDir(), CheckpointDir: c.ckDir, - Pools: []config.PoolSpec{spec}, Mesh: &config.MeshConfig{NodeID: "a"}, - Volumes: []config.VolumeSpec{{Name: "data", Path: writeVolumeImage(t, "data", "original"), Writable: true}}, - } - backends := testBackends(t, cfg) - backends.Archives = c.reg - m, err := NewManager(t.Context(), cfg, newFakeEngine(), testSecrets(t), backends) - if err != nil { - t.Fatal(err) - } - c.renew(t, "a") - m.liftFence() - return m, c + c.volumes = []config.VolumeSpec{{Name: "data", Path: writeVolumeImage(t, "data", "original"), Writable: true}} + return c.node(t, "a", t.TempDir(), keepAnHour), c } func disableReplication(m *Manager) { diff --git a/sandboxd/server/exec.go b/sandboxd/server/exec.go index 470100e1f..bb8609e6c 100644 --- a/sandboxd/server/exec.go +++ b/sandboxd/server/exec.go @@ -94,7 +94,7 @@ func (s *Server) handleExec(w http.ResponseWriter, r *http.Request) { return true }) if err != nil && pid != 0 { - // a dropped connection only reaches a child that writes; a silent one needs the kill + // A dropped connection only reaches a child that writes; a silent one needs the kill if killErr := s.killExec(ctx, id, token, pid); killErr != nil { log.WithFunc("server.handleExec").Errorf(ctx, killErr, "kill exec pid %d in %s", pid, id) } diff --git a/sandboxd/server/exec_test.go b/sandboxd/server/exec_test.go index 922aa720f..afac8bf6c 100644 --- a/sandboxd/server/exec_test.go +++ b/sandboxd/server/exec_test.go @@ -228,8 +228,7 @@ func TestExecTimeoutStartsAfterWake(t *testing.T) { return relayEnd, nil }} srv := New("", "node:7777", mgr, dialer, nil, nil, nil, nil) - ts := httptest.NewServer(srv.Handler()) - t.Cleanup(func() { ts.Close(); srv.CloseRelays() }) + ts := serveTestServer(t, srv) if status, body := postExec(t, ts, `{"argv":["true"],"timeout_seconds":1}`); status != http.StatusOK { t.Fatalf("status %d, want 200: %s", status, body) diff --git a/sandboxd/server/port_test.go b/sandboxd/server/port_test.go index 89a38f776..0c2cd54ac 100644 --- a/sandboxd/server/port_test.go +++ b/sandboxd/server/port_test.go @@ -237,12 +237,7 @@ func newPortServer(t *testing.T, guest func(net.Conn)) (*httptest.Server, *Serve return relayEnd, nil }} srv := New("", "node:7777", mgr, &fakeDialer{}, nil, nil, nil, nil) - ts := httptest.NewServer(srv.Handler()) - t.Cleanup(func() { - srv.CloseRelays() - ts.Close() - }) - return ts, srv + return serveTestServer(t, srv), srv } func dialPortConn(t *testing.T, ts *httptest.Server, req string) net.Conn { diff --git a/sandboxd/server/preview.go b/sandboxd/server/preview.go index 853fd8af0..457dfa822 100644 --- a/sandboxd/server/preview.go +++ b/sandboxd/server/preview.go @@ -53,7 +53,7 @@ func NewPreviewServer(secret, base, owner string, mgr PreviewManager) *PreviewSe } p := &PreviewServer{secret: []byte(secret), base: base, owner: owner, mgr: mgr} p.transport = &http.Transport{ - // a pooled guest conn skips PreviewDial, and with it wakeResolved and the Transition lock + // A pooled guest conn skips PreviewDial, and with it wakeResolved and the Transition lock DisableKeepAlives: true, DialContext: func(ctx context.Context, _, addr string) (net.Conn, error) { id, portStr, err := net.SplitHostPort(addr) @@ -97,7 +97,7 @@ func (p *PreviewServer) serve(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid or expired preview token", http.StatusForbidden) return } - // the token names the owner by its advertise address at mint time; holding the claim is what counts + // The token names the owner by its advertise address at mint time; holding the claim is what counts if _, held := p.mgr.Sandbox(claims.ID); !held && claims.Owner != p.owner { if r.Header.Get(forwardedHeader) != "" { http.Error(w, "preview owner is not this node", http.StatusBadGateway) @@ -113,10 +113,10 @@ func (p *PreviewServer) proxyLocal(w http.ResponseWriter, r *http.Request, claim rp := &httputil.ReverseProxy{ Rewrite: func(pr *httputil.ProxyRequest) { pr.Out.URL.Scheme = "http" - // the synthetic host carries PreviewDial's target + // The synthetic host carries PreviewDial's target pr.Out.URL.Host = fmt.Sprintf("%s:%d", claims.ID, claims.Port) pr.Out.URL.Path = "/" + strings.TrimPrefix(pr.In.URL.Path, "/p/"+r.PathValue("token")+"/") - // the raw form drops the same two segments however the client spelled them, so %2F survives + // The raw form drops the same two segments however the client spelled them, so %2F survives raw := pr.In.URL.EscapedPath() for range 2 { if i := strings.IndexByte(raw[1:], '/'); i >= 0 { @@ -126,7 +126,7 @@ func (p *PreviewServer) proxyLocal(w http.ResponseWriter, r *http.Request, claim } } pr.Out.URL.RawPath = raw - // browser credentials for the preview domain must not reach guest code + // Browser credentials for the preview domain must not reach guest code pr.Out.Header.Del("Cookie") pr.Out.Header.Del("Authorization") }, diff --git a/sandboxd/server/reader_test.go b/sandboxd/server/reader_test.go index 08ed090fd..451f17026 100644 --- a/sandboxd/server/reader_test.go +++ b/sandboxd/server/reader_test.go @@ -350,6 +350,12 @@ func (l *logSink) String() string { return l.buf.String() } +func (l *logSink) Reset() { + l.mu.Lock() + defer l.mu.Unlock() + l.buf.Reset() +} + type sleepingEnv struct { ts *httptest.Server srv *Server diff --git a/sandboxd/server/relay.go b/sandboxd/server/relay.go index 09f4cd869..8bd0a9275 100644 --- a/sandboxd/server/relay.go +++ b/sandboxd/server/relay.go @@ -21,7 +21,7 @@ const ( // drainGrace bounds how long a finished silkd relay waits for the client to close. drainGrace = 30 * time.Second - // the keepAlive trio fails a relay whose client vanished without a FIN in about two minutes. + // The keepAlive trio fails a relay whose client vanished without a FIN in about two minutes. keepAliveIdle = 60 * time.Second keepAliveInterval = 15 * time.Second keepAliveCount = 4 @@ -123,7 +123,7 @@ func (s *Server) splice(client net.Conn, clientR io.Reader, guest net.Conn, hell } defer release() - // the http server may have armed a header-read deadline on this conn + // The http server may have armed a header-read deadline on this conn _ = client.SetDeadline(time.Time{}) keepAlive(client) if _, err := client.Write(hello); err != nil { @@ -221,7 +221,7 @@ func copyBoth(client net.Conn, clientR io.Reader, guest net.Conn, ends relayEnds defer close(done) _, err := utils.CopyIn(guest, clientR, noSplice) if err != nil { - // the client died rather than half-closing, so nothing is left to answer it + // The client died rather than half-closing, so nothing is left to answer it _ = guest.Close() return } diff --git a/sandboxd/server/relay_test.go b/sandboxd/server/relay_test.go index 23490d69b..69a3e09f2 100644 --- a/sandboxd/server/relay_test.go +++ b/sandboxd/server/relay_test.go @@ -44,15 +44,7 @@ func TestCloseRelaysRefusesLateRelays(t *testing.T) { }) srv.CloseRelays() - conn, err := net.Dial("tcp", ts.Listener.Addr().String()) - if err != nil { - t.Fatalf("dial: %v", err) - } - defer conn.Close() - _ = conn.SetDeadline(time.Now().Add(3 * time.Second)) - if _, err := io.WriteString(conn, agentRequest); err != nil { - t.Fatalf("write request: %v", err) - } + conn := dialPortConn(t, ts, agentRequest) if _, err := io.Copy(io.Discard, conn); err != nil { t.Errorf("late relay not refused promptly: %v", err) } @@ -106,16 +98,7 @@ func TestRelayDeliversPipelinedBytes(t *testing.T) { _, _ = io.WriteString(c, line) }) - conn, err := net.Dial("tcp", ts.Listener.Addr().String()) - if err != nil { - t.Fatalf("dial: %v", err) - } - defer conn.Close() - _ = conn.SetDeadline(time.Now().Add(3 * time.Second)) - if _, err = io.WriteString(conn, agentRequest+execFrame); err != nil { - t.Fatalf("write: %v", err) - } - r := bufio.NewReader(conn) + r := bufio.NewReader(dialPortConn(t, ts, agentRequest+execFrame)) readStatus101(t, r) got, err := r.ReadString('\n') if err != nil { @@ -150,12 +133,7 @@ func TestRelayEndsWhenItsClaimIsFenced(t *testing.T) { return relayEnd, nil }} srv := New("", "node:7777", &fakeManager{fenced: fenced}, dialer, nil, nil, nil, nil) - ts := httptest.NewServer(srv.Handler()) - t.Cleanup(func() { - srv.CloseRelays() - ts.Close() - }) - return upgradeConn(t, ts) + return upgradeConn(t, serveTestServer(t, srv)) } fenced := make(chan struct{}) conn, r := relayWithFence(fenced) @@ -173,11 +151,9 @@ func TestRelayEndsWhenItsClaimIsFenced(t *testing.T) { } func TestWakeGuestLogsAClientCancelBelowError(t *testing.T) { - logs := &bytes.Buffer{} + logs := captureLogs(t) global := log.GetGlobalLogger() - saved := *global - *global = zerolog.New(logs).Level(zerolog.InfoLevel) - t.Cleanup(func() { *global = saved }) + *global = global.Level(zerolog.InfoLevel) dialer := &fakeDialer{dial: func(ctx context.Context, _ string) (net.Conn, error) { return nil, ctx.Err() }} srv := New("", "node:7777", &fakeManager{}, dialer, nil, nil, nil, nil) @@ -187,7 +163,7 @@ func TestWakeGuestLogsAClientCancelBelowError(t *testing.T) { if _, _, _, ok := srv.wakeGuest(canceled, w, "sb_1", "tok", srv.mgr.WakeAgentSocket); ok || w.Code != http.StatusBadGateway { t.Fatalf("canceled dial: ok %v status %d, want a refused 502", ok, w.Code) } - if logs.Len() != 0 { + if logs.String() != "" { t.Errorf("a client cancel logged %q, want nothing at info level", logs.String()) } @@ -279,25 +255,12 @@ func newRelayServer(t *testing.T, guest func(net.Conn)) (*httptest.Server, *Serv return relayEnd, nil }} srv := New("", "node:7777", &fakeManager{}, dialer, nil, nil, nil, nil) - ts := httptest.NewServer(srv.Handler()) - t.Cleanup(func() { - srv.CloseRelays() - ts.Close() - }) - return ts, srv + return serveTestServer(t, srv), srv } func upgradeConn(t *testing.T, ts *httptest.Server) (net.Conn, *bufio.Reader) { t.Helper() - conn, err := net.Dial("tcp", ts.Listener.Addr().String()) - if err != nil { - t.Fatalf("dial: %v", err) - } - t.Cleanup(func() { _ = conn.Close() }) - _ = conn.SetDeadline(time.Now().Add(3 * time.Second)) - if _, err := io.WriteString(conn, agentRequest); err != nil { - t.Fatalf("write request: %v", err) - } + conn := dialPortConn(t, ts, agentRequest) r := bufio.NewReader(conn) readStatus101(t, r) return conn, r diff --git a/sandboxd/server/server_test.go b/sandboxd/server/server_test.go index 4784abe4e..eab37bbd3 100644 --- a/sandboxd/server/server_test.go +++ b/sandboxd/server/server_test.go @@ -1047,7 +1047,7 @@ func TestForkFlow(t *testing.T) { ts := newTestServer(t, "sekret", mgr, nil) - post := func(auth, body string) *http.Response { + post := func(t *testing.T, auth, body string) *http.Response { req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, ts.URL+"/v1/sandboxes/sb_1/fork", strings.NewReader(body)) if err != nil { t.Fatalf("request: %v", err) @@ -1062,7 +1062,7 @@ func TestForkFlow(t *testing.T) { return resp } - resp := post("Bearer sekret", `{"token":"tok","count":2,"ttl_seconds":60}`) + resp := post(t, "Bearer sekret", `{"token":"tok","count":2,"ttl_seconds":60}`) defer resp.Body.Close() if resp.StatusCode != http.StatusOK { t.Fatalf("status %d, want 200", resp.StatusCode) @@ -1086,7 +1086,7 @@ func TestForkFlow(t *testing.T) { {"sandbox token is no api token", "Bearer tok", `{"token":"tok","count":1}`, http.StatusUnauthorized}, } { t.Run(tt.name, func(t *testing.T) { - resp := post(tt.auth, tt.body) + resp := post(t, tt.auth, tt.body) defer resp.Body.Close() if resp.StatusCode != tt.want { t.Errorf("status %d, want %d", resp.StatusCode, tt.want) @@ -1120,7 +1120,7 @@ func TestPromoteAndDeleteTemplateFlow(t *testing.T) { } ts := newTestServer(t, "sekret", mgr, nil) - promote := func(auth, body string) (int, PromoteResponse) { + promote := func(t *testing.T, auth, body string) (int, PromoteResponse) { req, _ := http.NewRequestWithContext(t.Context(), http.MethodPost, ts.URL+"/v1/sandboxes/sb_1/promote", strings.NewReader(body)) if auth != "" { req.Header.Set("Authorization", auth) @@ -1149,7 +1149,7 @@ func TestPromoteAndDeleteTemplateFlow(t *testing.T) { {"missing api token", "", `{"token":"tok","template":"tpl:x"}`, http.StatusUnauthorized}, } { t.Run("promote/"+tt.name, func(t *testing.T) { - got, result := promote(tt.auth, tt.body) + got, result := promote(t, tt.auth, tt.body) if got != tt.want { t.Errorf("status %d, want %d", got, tt.want) } @@ -2064,6 +2064,9 @@ func TestCheckpointClaimRedirectsToOwner(t *testing.T) { if got.ID != "" { t.Errorf("id = %q, want empty: a redirect and a delivered sandbox are mutually exclusive", got.ID) } + if mgr.healCalls != 0 { + t.Errorf("heal called %d times, want 0: a known owner must redirect, not heal", mgr.healCalls) + } } func TestCheckpointClaimNoRedirectResolvesLocally(t *testing.T) { @@ -2093,29 +2096,6 @@ func TestCheckpointClaimNoOwnersIs404(t *testing.T) { } } -func TestCheckpointClaimRedirectBeatsHeal(t *testing.T) { - mgr := &fakeManager{} - prober := &fakeProber{owners: []string{"owner-a:7777"}} - ts := newProberTestServer(t, "sekret", mgr, prober) - - resp := postJSON(t, ts.URL+"/v1/checkpoints/ck_00000000000000aa/claim", "sekret", `{}`) - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - t.Fatalf("status = %d, want 200 carrying a redirect", resp.StatusCode) - } - var got ClaimResponse - if err := json.UnmarshalRead(resp.Body, &got); err != nil { - t.Fatalf("decode: %v", err) - } - if len(got.Redirect) != 1 || got.Redirect[0] != "owner-a:7777" { - t.Errorf("redirect = %v, want [owner-a:7777]", got.Redirect) - } - if mgr.healCalls != 0 { - t.Errorf("heal called %d times, want 0: a known owner must redirect, not heal", mgr.healCalls) - } -} - func TestCheckpointClaimFallsBackToHealWhenNoOwner(t *testing.T) { mgr := &fakeManager{ healCheckpoint: func(ckptID string) (*types.Sandbox, error) { diff --git a/sandboxd/server/tenants_test.go b/sandboxd/server/tenants_test.go index 4a3a26623..d48f813d7 100644 --- a/sandboxd/server/tenants_test.go +++ b/sandboxd/server/tenants_test.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "net/http" - "net/http/httptest" "strings" "testing" "time" @@ -113,8 +112,7 @@ func TestRootFindsARemovedTenantsRemoteTemplate(t *testing.T) { types.TemplateGossipHash(hash, types.TemplateRootScope): {"peer:7777"}, }} srv := New("root", "node:7777", &fakeManager{}, &fakeDialer{}, placer, nil, nil, nil) - ts := httptest.NewServer(srv.Handler()) - t.Cleanup(ts.Close) + ts := serveTestServer(t, srv) for _, tt := range []struct{ method, path, body string }{ {http.MethodPost, "/v1/claim", `{"template":"tpl"}`}, {http.MethodPost, "/v1/claim", `{"template":"tpl","volumes":[{"name":"imagenet"}]}`}, @@ -145,8 +143,7 @@ func TestConfigReloadIsRootOnlyAndNamesWhatChanged(t *testing.T) { mgr := &fakeManager{tenants: []tenants.TenantSpec{{Name: "acme", Token: "acme-tok"}}} srv := New("root", "node:7777", mgr, &fakeDialer{}, nil, nil, nil, nil) srv.SetConfigReload(reload) - ts := httptest.NewServer(srv.Handler()) - t.Cleanup(ts.Close) + ts := serveTestServer(t, srv) resp := doReq(t, http.MethodPost, ts.URL+"/v1/config/reload", "acme-tok", "") _ = resp.Body.Close() diff --git a/sandboxd/store/digest_test.go b/sandboxd/store/digest_test.go index 1ab596a4a..773e01de2 100644 --- a/sandboxd/store/digest_test.go +++ b/sandboxd/store/digest_test.go @@ -41,20 +41,10 @@ func TestAssembleDigestVectors(t *testing.T) { } } -func TestAssembleDigestUsesChunkOffsetOrder(t *testing.T) { +func TestAssembleDigestMultiChunkVector(t *testing.T) { first := sha256.Sum256(bytes.Repeat([]byte("a"), int(DigestChunkSize))) second := sha256.Sum256([]byte("b")) - completed := []struct { - index int - sum [sha256.Size]byte - }{ - {index: 1, sum: second}, - {index: 0, sum: first}, - } - chunks := make([][sha256.Size]byte, len(completed)) - for _, result := range completed { - chunks[result.index] = result.sum - } + chunks := [][sha256.Size]byte{first, second} got, err := AssembleDigest([]DigestFile{{Path: "big.bin", Size: DigestChunkSize + 1, Chunks: chunks}}) if err != nil { diff --git a/sandboxd/store/dir/dir.go b/sandboxd/store/dir/dir.go index 92ca8ccc7..fc70fbf85 100644 --- a/sandboxd/store/dir/dir.go +++ b/sandboxd/store/dir/dir.go @@ -102,6 +102,14 @@ func (d *Store) FetchFile(ctx context.Context, id, rel, dst string) error { return err } +func (d *Store) ReadDigest(ctx context.Context, id string) (string, error) { + meta, err := d.ReadMeta(ctx, id) + if err != nil { + return "", err + } + return d.readDigest(id, meta) +} + func (d *Store) ReadMeta(_ context.Context, id string) ([]byte, error) { raw, err := os.ReadFile(filepath.Join(d.root, id, store.MetaFile)) //nolint:gosec // id pinned by the instance idRe before any call if errors.Is(err, fs.ErrNotExist) { @@ -226,7 +234,7 @@ func (d *Store) publish(_ context.Context, staging, id, digest string) error { switch { case errors.Is(statErr, fs.ErrNotExist): stagedExport := filepath.Join(staging, store.ExportDir) - // make the generation fresh before a peer can observe it without committed meta. + // Make the generation fresh before a peer can observe it without committed meta. if err := os.Chtimes(stagedExport, now, now); err != nil { return err } @@ -244,7 +252,7 @@ func (d *Store) publish(_ context.Context, staging, id, digest string) error { if readErr != nil && !errors.Is(readErr, fs.ErrNotExist) { return readErr } - // a sweep may already have selected an expired path for removal. + // A sweep may already have selected an expired path for removal. if time.Since(genInfo.ModTime()) >= store.GenerationGrace { return fmt.Errorf("generation %s expired before commit; retry after sweep", filepath.Base(genDir)) } @@ -256,7 +264,7 @@ func (d *Store) publish(_ context.Context, staging, id, digest string) error { _ = d.sweepGenerations(id) return os.RemoveAll(staging) } - // age the previous current generation from supersession, not publication. + // Age the previous current generation from supersession, not publication. if err := touchCurrentGeneration(final, now); err != nil { return err } @@ -282,7 +290,7 @@ func (d *Store) sweepGenerations(id string) (err error) { return err default: defer func() { err = errors.Join(err, metaFile.Close()) }() - // the open file pins one inode across a concurrent meta rename. + // The open file pins one inode across a concurrent meta rename. meta, readErr := io.ReadAll(metaFile) if readErr != nil { return readErr diff --git a/sandboxd/store/peer/broadcast.go b/sandboxd/store/peer/broadcast.go index 8176f8c8c..4040cde7e 100644 --- a/sandboxd/store/peer/broadcast.go +++ b/sandboxd/store/peer/broadcast.go @@ -1,11 +1,11 @@ package peer import ( - "cmp" "context" "fmt" "io" "net/http" + "slices" "sync" "time" @@ -19,19 +19,17 @@ const ( // Broadcaster fans a checkpoint delete out to every peer so a healed copy does not outlive it. type Broadcaster struct { - // a nil Client uses a default with a short timeout: one wedged peer must not hold up the fan-out. - Client *http.Client - Peers func() []string - Token string + Peers func() []string + Token string } // Delete sends DELETE ?no_forward=1 to every peer in parallel; failures are logged, not returned. func (b *Broadcaster) Delete(ctx context.Context, id string) { - addrs := dedupAddrs(b.Peers()) + addrs := slices.Compact(slices.Sorted(slices.Values(b.Peers()))) if len(addrs) == 0 { return } - client := cmp.Or(b.Client, &http.Client{Timeout: defaultDeleteClientTimeout}) + client := &http.Client{Timeout: defaultDeleteClientTimeout} logger := log.WithFunc("peer.Broadcaster.Delete") var wg sync.WaitGroup for _, addr := range addrs { diff --git a/sandboxd/store/peer/peer.go b/sandboxd/store/peer/peer.go index 41c2929a1..8ac240855 100644 --- a/sandboxd/store/peer/peer.go +++ b/sandboxd/store/peer/peer.go @@ -1,7 +1,6 @@ package peer import ( - "cmp" "context" "errors" "fmt" @@ -24,9 +23,6 @@ type Validate func(staging string) error type Healer struct { owners Owners puller Puller - - // budget overrides healBudget; a test seam, 30 minutes is unwaitable. - budget time.Duration } func NewHealer(owners Owners, puller Puller) *Healer { @@ -42,18 +38,17 @@ func (h *Healer) Pull(ctx context.Context, id, staging string, validate Validate if len(addrs) == 0 { return store.ErrNotFound } - budget := cmp.Or(h.budget, healBudget) - // a client hanging up must not abandon a started pull; the budget bounds it. - ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), budget) + // A client hanging up must not abandon a started pull; the budget bounds it. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), healBudget) defer cancel() - return h.pullFrom(ctx, id, staging, addrs, budget, validate) + return h.pullFrom(ctx, id, staging, addrs, validate) } -func (h *Healer) pullFrom(ctx context.Context, id, staging string, addrs []string, budget time.Duration, validate Validate) error { - perOwner := budget / time.Duration(len(addrs)) +func (h *Healer) pullFrom(ctx context.Context, id, staging string, addrs []string, validate Validate) error { + perOwner := healBudget / time.Duration(len(addrs)) var errs []error for _, addr := range addrs { - // one peer's rejected or partial transfer must not linger into the next's. + // One peer's rejected or partial transfer must not linger into the next's. if err := utils.RemoveDirEntries(staging, nil); err != nil { return fmt.Errorf("reset staging: %w", err) } diff --git a/sandboxd/store/peer/peer_test.go b/sandboxd/store/peer/peer_test.go index 5761e4e80..addf39785 100644 --- a/sandboxd/store/peer/peer_test.go +++ b/sandboxd/store/peer/peer_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "sync" "testing" + "testing/synctest" "time" "github.com/cocoonstack/sandbox/sandboxd/store" @@ -145,18 +146,17 @@ func TestPullValidateRejectsEveryOwner(t *testing.T) { } func TestPullBudgetBoundsSlowOwners(t *testing.T) { - puller := &sleepyPuller{sleep: 300 * time.Millisecond} - h := NewHealer(func(string) []string { return []string{"peer-a:7777", "peer-b:7777"} }, puller) - h.budget = 200 * time.Millisecond - - start := time.Now() - err := h.Pull(t.Context(), testID, t.TempDir(), nil) - if elapsed := time.Since(start); elapsed > 500*time.Millisecond { - t.Errorf("Pull took %v, want bounded near the %v budget", elapsed, h.budget) - } - if err == nil { - t.Error("Pull with two owners that never finish in time should fail, not silently succeed") - } + synctest.Test(t, func(t *testing.T) { + h := NewHealer(func(string) []string { return []string{"peer-a:7777", "peer-b:7777"} }, &sleepyPuller{sleep: 2 * healBudget}) + start := time.Now() + err := h.Pull(t.Context(), testID, t.TempDir(), nil) + if elapsed := time.Since(start); elapsed != healBudget { + t.Errorf("Pull took %v, want exactly the %v budget", elapsed, healBudget) + } + if err == nil { + t.Error("Pull with two owners that never finish in time should fail, not silently succeed") + } + }) } type fakePuller struct { diff --git a/sandboxd/store/peer/probe.go b/sandboxd/store/peer/probe.go index d43ac5ca0..1b0362275 100644 --- a/sandboxd/store/peer/probe.go +++ b/sandboxd/store/peer/probe.go @@ -9,6 +9,7 @@ import ( "encoding/binary" "maps" "net/http" + "slices" "sync" "time" @@ -40,9 +41,7 @@ type redirectCacheEntry struct { // HTTPProber finds which peers hold a checkpoint by probing them on a cross-node miss. type HTTPProber struct { - // a nil Client uses a default 2-second timeout: a probe must never hang on a wedged peer. - Client *http.Client - Peers func() []string + Peers func() []string // ProbeKey signs and verifies HEAD probes; empty means an unencrypted mesh. ProbeKey []byte @@ -75,7 +74,7 @@ func (p *HTTPProber) Owners(ctx context.Context, id string) []string { // HealOwners is Owners' wide twin: a heal wants the largest source list, uncached. func (p *HTTPProber) HealOwners(ctx context.Context, id string) []string { v, _, _ := p.healFlight.Do(id, func() (any, error) { - // no grace: a heal must not stop early on a fast owner and hide a slower valid source. + // No grace: a heal must not stop early on a fast owner and hide a slower valid source. return p.fanOut(ctx, id, maxHealOwners, 0), nil }) return v.([]string) @@ -90,11 +89,11 @@ func (p *HTTPProber) Forget(id string) { // fanOut probes every peer concurrently, canceling stragglers once maxOwners have answered 200. func (p *HTTPProber) fanOut(ctx context.Context, id string, maxOwners int, grace time.Duration) []string { - addrs := dedupAddrs(p.Peers()) + addrs := slices.Compact(slices.Sorted(slices.Values(p.Peers()))) if len(addrs) == 0 { return nil } - client := cmp.Or(p.Client, &http.Client{Timeout: defaultProbeClientTimeout}) + client := &http.Client{Timeout: defaultProbeClientTimeout} ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), defaultProbeClientTimeout) defer cancel() @@ -109,7 +108,7 @@ func (p *HTTPProber) fanOut(ctx context.Context, id string, maxOwners int, grace } go func() { wg.Wait(); close(wins) }() - // grace opens only after the first win: a genuine miss must hear from every peer. + // Grace opens only after the first win: a genuine miss must hear from every peer. var owners []string var graceCh <-chan time.Time for { @@ -201,20 +200,6 @@ func currentBucket() int64 { return time.Now().Unix() / probeBucketSeconds } -// dedupAddrs drops repeated addresses so a stale peer list never probes the same node twice. -func dedupAddrs(addrs []string) []string { - seen := make(map[string]struct{}, len(addrs)) - out := make([]string, 0, len(addrs)) - for _, a := range addrs { - if _, ok := seen[a]; ok { - continue - } - seen[a] = struct{}{} - out = append(out, a) - } - return out -} - // probeOwner sends a probe MAC when probeKey is set; an empty key falls back to capability-only. func probeOwner(ctx context.Context, client *http.Client, probeKey []byte, addr, id string) bool { ctx, cancel := context.WithTimeout(ctx, probeTimeout) diff --git a/sandboxd/store/peer/transport.go b/sandboxd/store/peer/transport.go index 262dc9291..e5511fda4 100644 --- a/sandboxd/store/peer/transport.go +++ b/sandboxd/store/peer/transport.go @@ -24,7 +24,7 @@ import ( ) const ( - // generous for a guest memory image, but a wedged peer must fail so the next owner is tried. + // Generous for a guest memory image, but a wedged peer must fail so the next owner is tried. pullTimeout = 30 * time.Minute maxRecordBytes = 1 << 40 // 1 TiB @@ -49,7 +49,7 @@ type Puller interface { // HTTPPuller pulls records over sandboxd's control-plane HTTP port. type HTTPPuller struct { - // a nil Client uses a default with no overall timeout; the deadline comes from the context. + // A nil Client uses a default with no overall timeout; the deadline comes from the context. Client *http.Client Token string } diff --git a/sandboxd/store/s3/s3.go b/sandboxd/store/s3/s3.go index f7ae33587..58b7e0fc6 100644 --- a/sandboxd/store/s3/s3.go +++ b/sandboxd/store/s3/s3.go @@ -96,7 +96,7 @@ func New(ctx context.Context, cfg store.S3Config, stagingRoot string, idRe *rege if cfg.Bucket == "" { return nil, fmt.Errorf("s3 checkpoint store needs a bucket") } - // a prefix without its trailing slash hides every record from the delimiter listing. + // A prefix without its trailing slash hides every record from the delimiter listing. if cfg.Prefix != "" && !strings.HasSuffix(cfg.Prefix, "/") { cfg.Prefix += "/" } @@ -198,6 +198,11 @@ func (s *Store) ReadMeta(ctx context.Context, id string) ([]byte, error) { return meta, err } +func (s *Store) ReadDigest(ctx context.Context, id string) (string, error) { + _, digest, err := s.readMeta(ctx, id) + return digest, err +} + func (s *Store) Metas(ctx context.Context) ([]store.Record, error) { ids, err := s.recordIDs(ctx) if err != nil { @@ -381,7 +386,7 @@ func (s *Store) publish(ctx context.Context, staging, id, lineage string, digest if err = s.putObject(ctx, s.key(id, store.MetaFile), metaRaw, metadata); err != nil { return "", err } - // keep old generations: another node may have selected the previous meta. + // Keep old generations: another node may have selected the previous meta. if err := os.RemoveAll(staging); err != nil { return "", err } @@ -468,11 +473,8 @@ func (s *Store) fetchChunk(ctx context.Context, key string, f *os.File, size int return nil } -func (s *Store) key(id, rest string) string { - if rest == "" { - return s.prefix + id - } - return s.prefix + id + "/" + filepath.ToSlash(rest) +func (s *Store) key(id, file string) string { + return s.prefix + id + "/" + file } func (s *Store) manifestPrefix(id string) string { @@ -492,7 +494,7 @@ func (s *Store) chunkKey(lineage, sha string) string { } func (s *Store) recordIDs(ctx context.Context) ([]string, error) { - // delimiter listing yields one CommonPrefix per record instead of every export object. + // Delimiter listing yields one CommonPrefix per record instead of every export object. var ids []string p := awss3.NewListObjectsV2Paginator(s.client, &awss3.ListObjectsV2Input{ Bucket: &s.bucket, Prefix: &s.prefix, Delimiter: new("/"), @@ -598,7 +600,7 @@ func (s *Store) deleteKeys(ctx context.Context, keys []string) error { return fmt.Errorf("delete %d objects: %w", len(chunk), err) } for _, e := range out.Errors { - // strict backends error on deleting an absent key where AWS succeeds silently. + // Strict backends error on deleting an absent key where AWS succeeds silently. if code := aws.ToString(e.Code); code == "NoSuchKey" || code == "NoSuchVersion" { continue } diff --git a/sandboxd/store/s3/s3_test.go b/sandboxd/store/s3/s3_test.go index 8e3f84d22..661cf605a 100644 --- a/sandboxd/store/s3/s3_test.go +++ b/sandboxd/store/s3/s3_test.go @@ -143,6 +143,8 @@ func TestFetchOutlivesTheCallerThatStartedIt(t *testing.T) { metaKey := "ck/" + id + "/" + store.MetaFile fake := &fakeS3{objects: map[string][]byte{}, exportGate: make(chan struct{}), exportEntered: make(chan struct{}, 1)} st := newTestStore(t, fake) + release := sync.OnceFunc(func() { close(fake.exportGate) }) + t.Cleanup(release) publishRecord(t, st, id, []byte(`{"id":"`+id+`"}`), "payload") fake.resetRequests() @@ -169,10 +171,10 @@ func TestFetchOutlivesTheCallerThatStartedIt(t *testing.T) { t.Fatalf("first caller's Fetch = %v, want its own cancellation", err) } case <-time.After(5 * time.Second): - close(fake.exportGate) + release() t.Fatal("first caller's Fetch still blocked on the stalled download after it hung up") } - close(fake.exportGate) + release() if err := <-joined; err != nil { t.Fatalf("second caller's Fetch = %v after the first caller hung up, want the shared download", err) } @@ -461,7 +463,7 @@ func TestSparseExportStaysSparseThroughS3(t *testing.T) { t.Fatalf("fetched size = %v, %v, want %d", info, err, size) } if fileSum(t, got) != srcSum { - t.Errorf("fetched content differs from the source") + t.Error("fetched content differs from the source") } if blocks := allocatedBytes(t, got); blocks > uploaded+(1<<20) { t.Errorf("fetched file allocates %d bytes, want at most the %d uploaded", blocks, uploaded) @@ -605,7 +607,7 @@ func TestLineageDedupUploadsOnlyChangedChunks(t *testing.T) { t.Fatalf("Fetch: %v", err) } if fileSum(t, filepath.Join(dir, "disk.img")) != srcSum { - t.Errorf("fetched content differs from the source") + t.Error("fetched content differs from the source") } } @@ -785,8 +787,10 @@ func TestFetchFileDownloadsOnlyTheRequestedFile(t *testing.T) { func TestFetchFileInstallsOnlyACompleteDownload(t *testing.T) { const id = "ck_00000000000000aa" meta := []byte(`{"id":"` + id + `"}`) - fake := &fakeS3{objects: map[string][]byte{}, exportGate: make(chan struct{}), exportEntered: make(chan struct{}, 1)} + fake := &fakeS3{objects: map[string][]byte{}, exportGate: make(chan struct{}), exportEntered: make(chan struct{}, 1), gateOn: "/chunks/"} st := newTestStore(t, fake) + release := sync.OnceFunc(func() { close(fake.exportGate) }) + t.Cleanup(release) publishRecord(t, st, id, meta, "disk-bytes") dst := filepath.Join(t.TempDir(), "disk.img") @@ -798,7 +802,7 @@ func TestFetchFileInstallsOnlyACompleteDownload(t *testing.T) { if _, err := os.Stat(dst); !errors.Is(err, os.ErrNotExist) { t.Errorf("a download in flight is already visible at the destination: %v", err) } - close(fake.exportGate) + release() if err := <-fetched; err != nil { t.Fatalf("FetchFile: %v", err) } @@ -828,7 +832,7 @@ func TestFetchFileKeepsHoles(t *testing.T) { t.Fatalf("fetched size = %v, %v, want %d", info, err, size) } if fileSum(t, dst) != srcSum { - t.Errorf("fetched content differs from the source") + t.Error("fetched content differs from the source") } if blocks, limit := allocatedBytes(t, dst), 2*store.DigestChunkSize+1<<20; blocks > limit { t.Errorf("fetched file allocates %d bytes, want at most %d for two data chunks", blocks, limit) @@ -977,11 +981,12 @@ type fakeS3 struct { failDelete bool exportGate chan struct{} exportEntered chan struct{} + gateOn string } func (f *fakeS3) ServeHTTP(w http.ResponseWriter, r *http.Request) { key := strings.TrimPrefix(r.URL.Path, "/testbucket/") - if f.exportGate != nil && r.Method == http.MethodGet && strings.Contains(key, "/"+store.ExportDir) { + if f.exportGate != nil && r.Method == http.MethodGet && strings.Contains(key, cmp.Or(f.gateOn, "/"+store.ExportDir)) { select { case f.exportEntered <- struct{}{}: default: @@ -1145,8 +1150,8 @@ func (f *fakeS3) handleListObjects(w http.ResponseWriter, r *http.Request) { continue } if delim != "" { - if i := strings.Index(key[len(prefix):], delim); i >= 0 { - common := key[:len(prefix)+i+1] + if before, _, ok := strings.Cut(key[len(prefix):], delim); ok { + common := prefix + before + delim if !seen[common] { seen[common] = true result.CommonPrefixes = append(result.CommonPrefixes, commonPrefix{Prefix: common}) diff --git a/sandboxd/store/sparse_test.go b/sandboxd/store/sparse_test.go index 2a56e1af9..d2f5ac5c3 100644 --- a/sandboxd/store/sparse_test.go +++ b/sandboxd/store/sparse_test.go @@ -50,7 +50,7 @@ func TestScanFileEmitsEachNonZeroChunkOnce(t *testing.T) { want = append(want, sha256.Sum256(buf[:n])) } if !slices.Equal(digest.Chunks, want) { - t.Errorf("chunk sums differ from a dense read") + t.Error("chunk sums differ from a dense read") } } diff --git a/sandboxd/store/store.go b/sandboxd/store/store.go index edd0260db..bb7533286 100644 --- a/sandboxd/store/store.go +++ b/sandboxd/store/store.go @@ -70,6 +70,7 @@ type Store interface { FetchFile(ctx context.Context, id, rel, dst string) error // ReadMeta returns a record's metadata, or an error when the record does not exist. ReadMeta(ctx context.Context, id string) ([]byte, error) + ReadDigest(ctx context.Context, id string) (string, error) // Metas lists the metadata and digest of every record in this instance's id namespace. Metas(ctx context.Context) ([]Record, error) // SetLabels replaces a record's labels; nil removes them. The caller holds the record lock. diff --git a/sdk/go/port.go b/sdk/go/port.go index ab07c6ae7..c8fdedc00 100644 --- a/sdk/go/port.go +++ b/sdk/go/port.go @@ -144,7 +144,7 @@ func (s *Sandbox) openStream(ctx context.Context, req wire.Request) (*PortConn, func (s *Sandbox) proxyConn(ctx context.Context, local net.Conn, port uint16) { defer func() { _ = local.Close() }() - // the copy can sit in local.Read forever; only closing the conn unblocks it, so ctx teardown reaches local too + // The copy can sit in local.Read forever; only closing the conn unblocks it, so ctx teardown reaches local too unarm := context.AfterFunc(ctx, func() { _ = local.Close() }) defer unarm() guest, err := s.DialPort(ctx, port) diff --git a/sdk/go/proc.go b/sdk/go/proc.go index e2aaa180e..7bbca2d6a 100644 --- a/sdk/go/proc.go +++ b/sdk/go/proc.go @@ -69,7 +69,7 @@ func (s *Sandbox) drainProc(ctx context.Context, req wire.Request, stdout, stder } defer l.close() _, code, exited, err := pumpStdio(ctx, conn, stdout, stderr) - // logs closes with done after the exit frame + // Logs closes with done after the exit frame if _, replay := req.(*wire.Logs); replay && exited && err == nil { err = terminalErr(ctx, conn) } diff --git a/sdk/go/sandbox.go b/sdk/go/sandbox.go index 0a3ceb34b..cf28fd302 100644 --- a/sdk/go/sandbox.go +++ b/sdk/go/sandbox.go @@ -119,7 +119,7 @@ func (s *Sandbox) Run(ctx context.Context, cmd Cmd) (int, error) { stdinDone := make(chan struct{}) if cmd.Stdin == nil { - // a guest that already answered and closed fails this send; the frames it sent still come back below + // A guest that already answered and closed fails this send; the frames it sent still come back below _ = conn.Send(wire.StdinClose{}) close(stdinDone) } else { @@ -130,7 +130,7 @@ func (s *Sandbox) Run(ctx context.Context, cmd Cmd) (int, error) { } pid, code, exited, err := pumpStdio(ctx, conn, cmd.Stdout, cmd.Stderr) - // a pump still parked in Read would write into the next RPC, so its connection is not kept + // A pump still parked in Read would write into the next RPC, so its connection is not kept select { case <-stdinDone: err = l.done(err) diff --git a/sdk/go/sandbox_test.go b/sdk/go/sandbox_test.go index 72861ffc9..d5b596d12 100644 --- a/sdk/go/sandbox_test.go +++ b/sdk/go/sandbox_test.go @@ -98,7 +98,7 @@ func TestRunCancelKillsTheCommandItStarted(t *testing.T) { case *wire.Info: _, _ = io.WriteString(conn, `{"type":"info","version":"test","proto":2}`+"\n") case *wire.Exec: - _, _ = io.WriteString(conn, `{"type":"started","pid":7}`+"\n") + _, _ = io.WriteString(conn, `{"type":"started","pid":7}`+"\n"+`{"type":"stdout","data":"NDIK"}`+"\n") _, _ = io.Copy(io.Discard, r) return case *wire.Kill: @@ -108,10 +108,10 @@ func TestRunCancelKillsTheCommandItStarted(t *testing.T) { } })) - ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + ctx, cancel := context.WithCancel(t.Context()) defer cancel() - if _, err := sb.Run(ctx, Cmd{Argv: []string{"sleep", "300"}}); !errors.Is(err, context.DeadlineExceeded) { - t.Fatalf("got %v, want deadline exceeded", err) + if _, err := sb.Run(ctx, Cmd{Argv: []string{"sleep", "300"}, Stdout: onWrite(cancel)}); !errors.Is(err, context.Canceled) { + t.Fatalf("got %v, want canceled", err) } select { case pid := <-killed: @@ -311,3 +311,10 @@ func legacySandbox(t *testing.T, ts *httptest.Server) *Sandbox { sb.proto.Store(1) return sb } + +type onWrite func() + +func (f onWrite) Write(p []byte) (int, error) { + f() + return len(p), nil +} diff --git a/sdk/go/upgrade.go b/sdk/go/upgrade.go index 6b8998c69..117a69e93 100644 --- a/sdk/go/upgrade.go +++ b/sdk/go/upgrade.go @@ -2,6 +2,7 @@ package sandbox import ( "bufio" + "cmp" "context" "crypto/tls" "fmt" @@ -77,7 +78,7 @@ func (c *Client) dialAgent(ctx context.Context, addr, id, token string) (*upgrad secured := tls.Client(raw, cfg) if err = secured.HandshakeContext(ctx); err != nil { _ = raw.Close() - return nil, fmt.Errorf("agent TLS handshake: %w", err) + return nil, fmt.Errorf("agent TLS handshake: %w", cmp.Or(ctx.Err(), err)) } conn = secured } @@ -88,7 +89,7 @@ func (c *Client) dialAgent(ctx context.Context, addr, id, token string) (*upgrad if scheme != httpsScheme { return u, nil } - // a TLS edge may be a proxy that drops bytes sent ahead of the 101 + // A TLS edge may be a proxy that drops bytes sent ahead of the 101 if _, err = u.Write(nil); err != nil { _ = raw.Close() return nil, fmt.Errorf("write upgrade request: %w", err) diff --git a/sdk/openai/cocoonsandbox_openai/adapter.py b/sdk/openai/cocoonsandbox_openai/adapter.py index a0caed381..2e82b321e 100644 --- a/sdk/openai/cocoonsandbox_openai/adapter.py +++ b/sdk/openai/cocoonsandbox_openai/adapter.py @@ -104,7 +104,7 @@ async def _exec_internal(self, *command: str | Path, timeout: float | None = Non stdout, stderr = bytearray(), bytearray() def run() -> int: - # the SDK's wall clock cuts the connection, which kills the command, and raises TimeoutError + # the SDK's wall clock cuts the connection, kills the reported pid, and raises TimeoutError return sb.run(argv, on_stdout=stdout.extend, on_stderr=stderr.extend, timeout=timeout) code = await asyncio.to_thread(run) diff --git a/sdk/python/cocoonsandbox/sandbox.py b/sdk/python/cocoonsandbox/sandbox.py index b3368d440..347bd3745 100644 --- a/sdk/python/cocoonsandbox/sandbox.py +++ b/sdk/python/cocoonsandbox/sandbox.py @@ -140,6 +140,8 @@ def run( """Streams raw output bytes and returns the exit code; timeout bounds the call and kills what it cuts.""" if timeout is not None and timeout <= 0: raise ValueError("timeout must be positive") + if stdin and not isinstance(stdin, (bytes, bytearray, memoryview)): + raise TypeError("stdin must be bytes") deadline = None if timeout is None else time.monotonic() + timeout expired = threading.Event() started: deque[int] = deque(maxlen=1) diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index bd44278f1..f796c33fc 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -5,7 +5,7 @@ from http.server import HTTPServer, ThreadingHTTPServer import pytest -from conftest import FakeNode, sandbox_at +from conftest import FakeNode, sandbox_at, wait_until from cocoonsandbox import APIError, Client, Template @@ -14,11 +14,17 @@ class KeptNode(FakeNode): protocol_version = "HTTP/1.1" conns = 0 drop_after_reply = False + closed = 0 def setup(self): super().setup() KeptNode.conns += 1 + def finish(self): + super().finish() + self.connection.close() + KeptNode.closed += 1 + def _reply(self, code, payload): super()._reply(code, payload) self.close_connection = self.drop_after_reply @@ -374,7 +380,7 @@ def test_a_parked_connection_the_node_closed_is_not_reused(kept_node, fd_floor): KeptNode.drop_after_reply = True client = Client(kept_node) client.new("rt:24.04") - time.sleep(0.1) + wait_until(lambda: KeptNode.closed == 1, "the node never closed the connection") assert client.new("rt:24.04").id == "sb_1" assert KeptNode.conns == 2 @@ -382,7 +388,7 @@ def test_a_parked_connection_the_node_closed_is_not_reused(kept_node, fd_floor): @pytest.fixture def kept_node(): server = ThreadingHTTPServer(("127.0.0.1", 0), KeptNode) - KeptNode.routes, KeptNode.conns, KeptNode.drop_after_reply = {}, 0, False + KeptNode.routes, KeptNode.conns, KeptNode.closed, KeptNode.drop_after_reply = {}, 0, 0, False threading.Thread(target=server.serve_forever, daemon=True).start() yield f"127.0.0.1:{server.server_port}" server.shutdown() diff --git a/sdk/python/tests/test_fixtures.py b/sdk/python/tests/test_fixtures.py index 2f9b4f2a7..2d30b607e 100644 --- a/sdk/python/tests/test_fixtures.py +++ b/sdk/python/tests/test_fixtures.py @@ -3,6 +3,7 @@ byte-for-byte (up to key order) through frames.encode_request, and every response fixture must decode.""" +import base64 import json import pathlib @@ -22,7 +23,7 @@ def fixture_files(prefix): @pytest.mark.parametrize("path", fixture_files("req_"), ids=lambda p: p.stem) def test_request_round_trip(path): want = json.loads(path.read_text()) - fields = {k: v for k, v in want.items() if k not in ("v", "op")} + fields = {k: base64.b64decode(v) if k == "data" else v for k, v in want.items() if k not in ("v", "op")} got = json.loads(frames.encode_request(want["op"], **fields)) assert got == want @@ -30,7 +31,8 @@ def test_request_round_trip(path): @pytest.mark.parametrize("path", fixture_files("resp_"), ids=lambda p: p.stem) def test_response_decodes(path): raw = path.read_text().encode() + want = json.loads(raw) frame = frames.decode_response(raw) - assert frame["type"] == json.loads(raw)["type"] - if "data" in json.loads(raw): - assert isinstance(frame["data"], bytes) + assert frame["type"] == want["type"] + if "data" in want: + assert frame["data"] == base64.b64decode(want["data"]) diff --git a/sdk/python/tests/test_keepalive.py b/sdk/python/tests/test_keepalive.py index 058307296..3b39e81a2 100644 --- a/sdk/python/tests/test_keepalive.py +++ b/sdk/python/tests/test_keepalive.py @@ -5,7 +5,6 @@ import json import socket import threading -import time import pytest from conftest import accept_upgrade, sandbox_at, wait_until @@ -161,22 +160,25 @@ def test_peer_hang_up_is_noticed_before_reuse(fd_floor): def test_run_timeout_bounds_the_proto_probe(): server = socket.create_server(("127.0.0.1", 0)) + cut = threading.Event() def serve(): conn, _ = server.accept() - accept_upgrade(conn) - time.sleep(3) - conn.close() + conn.settimeout(3) + with conn, contextlib.suppress(OSError): + reader = accept_upgrade(conn) + while reader.readline(): + pass + cut.set() threading.Thread(target=serve, daemon=True).start() sb = sandbox_at(f"127.0.0.1:{server.getsockname()[1]}") - started = time.monotonic() try: with pytest.raises(TimeoutError): sb.run(["true"], timeout=0.5) + assert cut.wait(1), "the unanswered info probe outlived the run timeout" finally: server.close() - assert time.monotonic() - started < 2, "the unanswered info probe outlived the run timeout" def test_close_drains_the_parked_connection(monkeypatch): diff --git a/sdk/python/tests/test_proc.py b/sdk/python/tests/test_proc.py index ad5f7eaa3..e3418488a 100644 --- a/sdk/python/tests/test_proc.py +++ b/sdk/python/tests/test_proc.py @@ -62,6 +62,20 @@ def test_run_pumps_stdin_while_reading_output(monkeypatch): assert blocking.sent[-1][0] == "stdin_close" +@pytest.mark.parametrize( + "call", + [lambda sb: sb.run(["cat"], stdin="hi"), lambda sb: sb.exec("cat", stdin="hi")], + ids=["run", "exec"], +) +def test_a_text_stdin_is_refused_before_dialing(monkeypatch, call): + sb = sandbox_at("127.0.0.1:1", keep_alive=0) + conn = StdinGatedConn([{"type": "exit", "code": 0}]) + monkeypatch.setattr(sb, "_dial", lambda deadline=None: conn) + with pytest.raises(TypeError, match="stdin must be bytes"): + call(sb) + assert conn.sent == [] + + class FakeConn: """Answers one RPC with a canned frame list.""" @@ -112,6 +126,23 @@ def recv(self): return super().recv() +class StdinGatedConn(FakeConn): + """A guest that answers only after stdin closes, as cat does.""" + + def __init__(self, frames): + super().__init__(frames) + self._closed = threading.Event() + + def send(self, op, **fields): + super().send(op, **fields) + if op == "stdin_close": + self._closed.set() + + def recv(self): + assert self._closed.wait(2), "stdin_close never sent" + return super().recv() + + def fake_sandbox(monkeypatch, frames): sb = sandbox_at("127.0.0.1:1", keep_alive=0) conn = FakeConn(frames) diff --git a/silkd/src/exec.rs b/silkd/src/exec.rs index cc81338eb..219ebefc5 100644 --- a/silkd/src/exec.rs +++ b/silkd/src/exec.rs @@ -87,7 +87,7 @@ where let pump = tokio::spawn(pump_out(Arc::clone(&proc), stdout, stderr, pump_fg)); let pump_abort = pump.abort_handle(); // stdin rides its own task so a slow write cannot stall output or reaping. - tokio::spawn(pump_stdin(stdin, client, req.detach)); + let stdin_task = tokio::spawn(pump_stdin(stdin, client, req.detach)); // record the reaped code before the drain: an abort mid-drain must publish the real code, not -1. let reaped: Arc> = Arc::new(OnceLock::new()); @@ -124,6 +124,7 @@ where // client gone: publish the terminal state the abort pre-empts, else an attacher waits forever. supervise.abort(); pump_abort.abort(); + stdin_task.abort(); let _ = supervise.await; if proc.exit_code().is_none() { proc.finish(reaped.get().copied().unwrap_or(-1)); @@ -132,6 +133,7 @@ where return Err(e); } let code = supervise.await.unwrap_or(-1); + stdin_task.abort(); table.remove_if(pid, &proc); proto::write_frame(out, &Response::Exit { code }).await } diff --git a/silkd/src/fs.rs b/silkd/src/fs.rs index 1ff43e2da..9870f2448 100644 --- a/silkd/src/fs.rs +++ b/silkd/src/fs.rs @@ -1,7 +1,7 @@ //! Filesystem verbs: the vsock boundary is the trust boundary, so paths are taken as-is. use std::io; -use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; +use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; use std::path::{Path, PathBuf}; use std::time::UNIX_EPOCH; @@ -212,24 +212,20 @@ fn tmp_name(path: &Path) -> PathBuf { PathBuf::from(tmp) } -/// Creates the temp file exclusively under its final mode; without an explicit mode an overwrite inherits the destination's bits. +/// Creates the temp file exclusively under its final mode; without an explicit mode an overwrite inherits the destination's bits, and under root always its owner. async fn create_tmp(path: &Path, mode: Option) -> io::Result<(PathBuf, fs::File)> { let path = path.to_path_buf(); let (tmp, file) = tokio::task::spawn_blocking(move || { - let effective = mode.or_else(|| { - std::fs::metadata(&path) - .ok() - .map(|meta| meta.permissions().mode() & 0o7777) - }); + let dest = std::fs::metadata(&path).ok(); + let effective = + mode.or_else(|| dest.as_ref().map(|meta| meta.permissions().mode() & 0o7777)); let tmp = tmp_name(&path); let file = std::fs::OpenOptions::new() .write(true) .create_new(true) .mode(effective.unwrap_or(0o666)) .open(&tmp)?; - if let Some(m) = effective - && let Err(e) = file.set_permissions(std::fs::Permissions::from_mode(m)) - { + if let Err(e) = apply_dest_attrs(&file, dest.as_ref(), effective) { let _ = std::fs::remove_file(&tmp); return Err(e); } @@ -240,6 +236,25 @@ async fn create_tmp(path: &Path, mode: Option) -> io::Result<(PathBuf, fs:: Ok((tmp, fs::File::from_std(file))) } +fn apply_dest_attrs( + file: &std::fs::File, + dest: Option<&std::fs::Metadata>, + mode: Option, +) -> io::Result<()> { + if let Some(dest) = dest + && nix::unistd::geteuid().is_root() + { + let tmp = file.metadata()?; + if (tmp.uid(), tmp.gid()) != (dest.uid(), dest.gid()) { + std::os::unix::fs::fchown(file, Some(dest.uid()), Some(dest.gid()))?; + } + } + if let Some(m) = mode { + file.set_permissions(std::fs::Permissions::from_mode(m))?; + } + Ok(()) +} + async fn commit_tmp(tmp: &Path, path: &Path) -> io::Result<()> { let outcome = fs::rename(tmp, path).await; if outcome.is_err() { diff --git a/silkd/src/net.rs b/silkd/src/net.rs index b68b4e764..976131c3c 100644 --- a/silkd/src/net.rs +++ b/silkd/src/net.rs @@ -39,7 +39,7 @@ pub fn routes_directly() -> bool { if !has_egress() { return false; } - // the host writes the file once per claim, so a present verdict latches; only absence re-reads. + // the host writes the verdict before the golden snapshot or on a cold boot, so a present verdict latches; only absence re-reads. let relay = match LANE.load(Ordering::Relaxed) { -1 => lane_verdict(LANE_FILE) .inspect(|&r| LANE.store(i8::from(r), Ordering::Relaxed)) diff --git a/silkd/src/proto.rs b/silkd/src/proto.rs index 9de79b9cd..71cfcdc1f 100644 --- a/silkd/src/proto.rs +++ b/silkd/src/proto.rs @@ -22,7 +22,7 @@ pub const PROTO_VERSION: u32 = 1; /// 2 serves RPCs back to back on one connection; 1 closed after one. pub const KEEP_ALIVE_PROTO: u32 = 2; -/// Chunk size for streaming a file back over `fs.read`. +/// Read size for exec, pty and session output. pub const READ_CHUNK: usize = 32 * 1024; /// Chunk size for bulk streams; stays under MAX_FRAME after base64. pub const BULK_CHUNK: usize = 256 * 1024; diff --git a/silkd/src/pty.rs b/silkd/src/pty.rs index f3dfe0c4d..64a680dc5 100644 --- a/silkd/src/pty.rs +++ b/silkd/src/pty.rs @@ -1,6 +1,6 @@ //! `pty.open` / `pty.resize`: a pty is a process, so it registers in the proc table and `ps`/`kill`/`attach`/`logs` apply unchanged. -use std::os::fd::{AsRawFd, OwnedFd}; +use std::os::fd::OwnedFd; use std::process::Stdio; use std::sync::Arc; use std::time::Duration; @@ -147,7 +147,7 @@ async fn pump( Ok(g) => g, Err(_) => { eof = true; continue } }; - match guard.try_io(|fd| sysutil::read_fd(fd.get_ref().as_raw_fd(), &mut buf)) { + match guard.try_io(|fd| nix::unistd::read(fd.get_ref(), &mut buf).map_err(Into::into)) { // a read of 0 (BSD) or any error (EIO on Linux) means the slave is fully closed. Ok(Ok(0)) | Ok(Err(_)) => eof = true, Ok(Ok(n)) => { @@ -199,7 +199,7 @@ async fn drain( let Ok(mut guard) = master.readable().await else { return; }; - match guard.try_io(|fd| sysutil::read_fd(fd.get_ref().as_raw_fd(), buf)) { + match guard.try_io(|fd| nix::unistd::read(fd.get_ref(), buf).map_err(Into::into)) { Ok(Ok(0)) | Ok(Err(_)) => return, Ok(Ok(n)) => { proc.emit_bytes(false, &buf[..n]); @@ -220,7 +220,7 @@ async fn drain( async fn write_all(master: &Master, mut data: &[u8]) -> std::io::Result<()> { while !data.is_empty() { let mut guard = master.writable().await?; - match guard.try_io(|fd| sysutil::write_fd(fd.get_ref().as_raw_fd(), data)) { + match guard.try_io(|fd| nix::unistd::write(fd.get_ref(), data).map_err(Into::into)) { Ok(Ok(n)) => data = &data[n..], Ok(Err(e)) => return Err(e), Err(_would_block) => continue, diff --git a/silkd/src/sysutil.rs b/silkd/src/sysutil.rs index cf6c98e42..5a2ef4773 100644 --- a/silkd/src/sysutil.rs +++ b/silkd/src/sysutil.rs @@ -9,6 +9,7 @@ use std::process::ExitStatus; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{LazyLock, Mutex}; +use nix::fcntl::{FcntlArg, OFlag, fcntl}; use tokio::io::unix::AsyncFd; use tokio::process::Command; @@ -52,7 +53,7 @@ pub fn rand_token() -> std::io::Result { .collect()) } -/// SIGKILLs the group led by `pgid`, so a session's external command dies with its shell; a synthetic id misses with ESRCH. +/// SIGKILLs the group led by `pgid`, so a session's external command dies with its shell. pub fn kill_group(pgid: u32) { if is_valid_pid(pgid) { send_signal(-(pgid as libc::pid_t), libc::SIGKILL); @@ -133,7 +134,8 @@ pub fn openpty(cols: u16, rows: u16) -> std::io::Result<(OwnedFd, OwnedFd)> { ws_ypixel: 0, }; let pty = nix::pty::openpty(&ws, None)?; - set_nonblocking(pty.master.as_raw_fd())?; + let flags = OFlag::from_bits_retain(fcntl(&pty.master, FcntlArg::F_GETFL)?); + fcntl(&pty.master, FcntlArg::F_SETFL(flags | OFlag::O_NONBLOCK))?; Ok((pty.master, pty.slave)) } @@ -167,26 +169,6 @@ pub fn adopt_controlling_tty(cmd: &mut Command) { } } -/// Reads from a raw fd, surfacing WouldBlock to the caller's readiness loop. -pub fn read_fd(fd: RawFd, buf: &mut [u8]) -> std::io::Result { - // SAFETY: fd is a live open fd; buf is a valid mutable slice of buf.len(). - let n = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), buf.len()) }; - if n < 0 { - return Err(std::io::Error::last_os_error()); - } - Ok(n as usize) -} - -/// Writes to a raw fd (the pty master), returning bytes written. -pub fn write_fd(fd: RawFd, buf: &[u8]) -> std::io::Result { - // SAFETY: fd is a live open fd; buf is a valid slice of buf.len(). - let n = unsafe { libc::write(fd, buf.as_ptr().cast(), buf.len()) }; - if n < 0 { - return Err(std::io::Error::last_os_error()); - } - Ok(n as usize) -} - /// Locks a std mutex, panicking on poisoning: silkd's critical sections never panic. #[allow(clippy::unwrap_used)] pub fn lock(m: &Mutex) -> std::sync::MutexGuard<'_, T> { @@ -331,19 +313,6 @@ fn lookup_user(user: &str) -> Result<(u32, u32, PathBuf), String> { } } -fn set_nonblocking(fd: RawFd) -> std::io::Result<()> { - // SAFETY: fd is open; F_GETFL/F_SETFL only read and set its flags. - let flags = unsafe { libc::fcntl(fd, libc::F_GETFL) }; - if flags < 0 { - return Err(std::io::Error::last_os_error()); - } - // SAFETY: same fd; F_SETFL only sets the just-read flags plus O_NONBLOCK. - if unsafe { libc::fcntl(fd, libc::F_SETFL, flags | libc::O_NONBLOCK) } < 0 { - return Err(std::io::Error::last_os_error()); - } - Ok(()) -} - #[cfg(test)] mod tests { use super::*; diff --git a/silkd/src/tree.rs b/silkd/src/tree.rs index 4b89cec4b..cdb041d43 100644 --- a/silkd/src/tree.rs +++ b/silkd/src/tree.rs @@ -80,7 +80,7 @@ pub async fn pull(w: &mut W, path: String) -> io::Result< } let status = child.wait().await?; let msg = err_task.await.unwrap_or_default(); - proto::subprocess_result(w, status.success(), "tar create", msg.as_bytes()).await + proto::subprocess_result(w, status.success(), "tar create", &msg).await } /// The caller owns removing `staging` on every path. @@ -116,7 +116,7 @@ where return proto::write_feed_error(w, fail).await; } if !status.success() { - return proto::subprocess_result(w, status.success(), "tar extract", msg.as_bytes()).await; + return proto::subprocess_result(w, status.success(), "tar extract", &msg).await; } // spawn_blocking: a big tree is thousands of local fs syscalls. let merged = tokio::task::spawn_blocking(move || merge_tree(&staging, &dest)) @@ -132,7 +132,7 @@ where /// Spawns tar draining stderr on its own task, so a full stderr pipe cannot deadlock. fn spawn_tar( mut cmd: Command, -) -> io::Result<(tokio::process::Child, tokio::task::JoinHandle)> { +) -> io::Result<(tokio::process::Child, tokio::task::JoinHandle>)> { let mut child = cmd.stderr(Stdio::piped()).kill_on_drop(true).spawn()?; let stderr = child .stderr @@ -187,7 +187,7 @@ fn is_dir(mode: stat::mode_t) -> bool { } /// Reads a child's stderr up to STDERR_CAP; tar's first error is the useful one. -async fn drain(mut stderr: tokio::process::ChildStderr) -> String { +async fn drain(mut stderr: tokio::process::ChildStderr) -> Vec { let mut out = Vec::new(); let mut buf = [0u8; 4096]; while let Ok(n) = stderr.read(&mut buf).await { @@ -198,7 +198,7 @@ async fn drain(mut stderr: tokio::process::ChildStderr) -> String { out.extend_from_slice(&buf[..(n.min(STDERR_CAP - out.len()))]); } } - String::from_utf8(out).unwrap_or_else(|e| String::from_utf8_lossy(e.as_bytes()).into_owned()) + out } #[cfg(test)] diff --git a/silkd/tests/exec_e2e.rs b/silkd/tests/exec_e2e.rs index db96a9f79..695798322 100644 --- a/silkd/tests/exec_e2e.rs +++ b/silkd/tests/exec_e2e.rs @@ -11,7 +11,7 @@ use silkd::server::State; use tokio::io::{AsyncReadExt, AsyncWriteExt, BufReader}; use common::{ - b64, connect, decode, exchange, next_frame, one, roundtrip, send, stdout_body, type_of, + b64, connect, exchange, frames_until, next_frame, one, roundtrip, send, stdout_body, type_of, }; async fn detached_pid(state: &Arc, script: &str) -> u64 { @@ -135,10 +135,7 @@ async fn daemonizer_exit_is_the_last_frame() { "exit must be last: {frames:?}" ); assert!( - !frames - .iter() - .filter(|f| type_of(f) == "stdout") - .any(|f| String::from_utf8_lossy(&decode(f)).contains("late")), + !stdout_body(&frames).contains("late"), "grandchild output must not leak after exit" ); } @@ -337,3 +334,73 @@ async fn disconnect_during_drain_publishes_real_exit_code() { ); assert_eq!(last["code"], 7, "real exit code, not a fabricated -1"); } + +#[tokio::test] +async fn stdin_held_by_a_forked_child_does_not_wedge_the_next_rpc() { + let state = Arc::new(State::new()); + let (mut cw, mut lines, handle) = connect(&state); + send( + &mut cw, + json!({ + "op": "exec", + "argv": ["/bin/sh", "-c", "exec 3<&0; sleep 8 <&3 3<&- & exit 0"] + }), + ) + .await; + let chunk = b64(&[b'x'; 32 * 1024]); + for _ in 0..30 { + send(&mut cw, json!({"op":"stdin","data":chunk})).await; + } + let frames = frames_until(&mut lines, |f| type_of(f) == "exit").await; + assert_eq!(frames.last().unwrap()["code"], 0, "{frames:?}"); + + send(&mut cw, json!({"op":"info"})).await; + let info = tokio::time::timeout(Duration::from_secs(3), next_frame(&mut lines)) + .await + .expect("the next RPC is wedged on a stdin nobody reads"); + assert_eq!(type_of(&info), "info"); + cw.shutdown().await.unwrap(); + handle.await.unwrap().unwrap(); +} + +#[tokio::test] +async fn a_client_gone_mid_output_releases_the_stdin_a_forked_child_holds() { + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("count"); + let state = Arc::new(State::new()); + let (mut cw, mut lines, _) = connect(&state); + send( + &mut cw, + json!({ + "op": "exec", + "argv": ["/bin/sh", "-c", "exec 3<&0; (sleep 1; wc -c <&3 >\"$OUT.tmp\"; mv \"$OUT.tmp\" \"$OUT\") >/dev/null 2>&1 & while :; do echo x; sleep 0.05; done"], + "env": {"OUT": out.to_str().unwrap()} + }), + ) + .await; + let chunk = b64(&[b'x'; 32 * 1024]); + for _ in 0..30 { + send(&mut cw, json!({"op":"stdin","data":chunk})).await; + } + frames_until(&mut lines, |f| type_of(f) == "stdout").await; + drop(lines); + drop(cw); + + let deadline = tokio::time::Instant::now() + Duration::from_secs(8); + while !out.exists() { + assert!( + tokio::time::Instant::now() < deadline, + "the forked child never saw stdin end" + ); + tokio::time::sleep(Duration::from_millis(50)).await; + } + let read: usize = std::fs::read_to_string(&out) + .unwrap() + .trim() + .parse() + .unwrap(); + assert!( + read < 256 * 1024, + "the forked child read {read} bytes: stdin kept flowing after its client left" + ); +} diff --git a/silkd/tests/find_e2e.rs b/silkd/tests/find_e2e.rs index fc497b10b..9625ea3a7 100644 --- a/silkd/tests/find_e2e.rs +++ b/silkd/tests/find_e2e.rs @@ -3,7 +3,7 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] mod common; -use std::os::unix::fs::PermissionsExt; +use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::sync::Arc; use serde_json::{Value, json}; @@ -183,9 +183,8 @@ async fn replace_skips_a_file_over_the_size_bound() { assert_eq!(replaced.len(), 1, "{frames:?}"); assert_eq!(replaced[0]["replacements"], 0); assert_eq!(type_of(frames.last().unwrap()), "done"); - assert_eq!( - tokio::fs::read_to_string(&big).await.unwrap().len(), - body.len(), + assert!( + tokio::fs::read_to_string(&big).await.unwrap() == body, "an oversized file must not be rewritten" ); } @@ -236,4 +235,30 @@ async fn replace_preserves_exec_bit() { .mode() & 0o777; assert_eq!(mode, 0o755, "exec bit stripped by replace"); + assert_eq!( + tokio::fs::read_to_string(&script).await.unwrap(), + "#!/bin/sh\nbar\n" + ); +} + +#[tokio::test] +#[ignore = "needs root"] +async fn replace_keeps_the_destination_owner_under_root() { + let dir = tempfile::tempdir().unwrap(); + let file = dir.path().join("owned.txt"); + std::fs::write(&file, "foo\n").unwrap(); + std::os::unix::fs::chown(&file, Some(1000), Some(1000)).unwrap(); + + exchange(&[json!({ + "op": "fs_replace", + "files": [file.to_str().unwrap()], + "pattern": "foo", + "replacement": "bar" + }) + .to_string()]) + .await; + + let meta = std::fs::metadata(&file).unwrap(); + assert_eq!((meta.uid(), meta.gid()), (1000, 1000)); + assert_eq!(std::fs::read_to_string(&file).unwrap(), "bar\n"); } diff --git a/silkd/tests/fs_e2e.rs b/silkd/tests/fs_e2e.rs index c71b39efe..193934ca0 100644 --- a/silkd/tests/fs_e2e.rs +++ b/silkd/tests/fs_e2e.rs @@ -3,11 +3,18 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] mod common; -use std::os::unix::fs::PermissionsExt; +use std::os::unix::fs::{MetadataExt, PermissionsExt}; -use serde_json::json; +use nix::sys::stat::Mode; +use serde_json::{Value, json}; -use common::{b64, exchange, payload, type_of}; +use common::{b64, data_frames, exchange, payload, type_of}; + +async fn fs_write(req: Value, body: &[u8]) -> Vec { + let mut lines = vec![req.to_string()]; + lines.extend(data_frames(body)); + exchange(&lines).await +} #[tokio::test] async fn write_then_read_roundtrips_bytes() { @@ -15,11 +22,10 @@ async fn write_then_read_roundtrips_bytes() { let path = dir.path().join("hello.txt"); let path = path.to_str().unwrap(); - let wrote = exchange(&[ - json!({"op":"fs_write","path":path,"mode":0o600}).to_string(), - json!({"op":"data","data":b64(b"silk file body")}).to_string(), - json!({"op":"data_end"}).to_string(), - ]) + let wrote = fs_write( + json!({"op":"fs_write","path":path,"mode":0o600}), + b"silk file body", + ) .await; assert_eq!( type_of(&wrote[0]), @@ -36,11 +42,10 @@ async fn write_then_read_roundtrips_bytes() { async fn list_reports_written_entry() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("a.txt"); - exchange(&[ - json!({"op":"fs_write","path":path.to_str().unwrap()}).to_string(), - json!({"op":"data","data":b64(b"abc")}).to_string(), - json!({"op":"data_end"}).to_string(), - ]) + fs_write( + json!({"op":"fs_write","path":path.to_str().unwrap()}), + b"abc", + ) .await; let listed = @@ -84,11 +89,10 @@ async fn list_batches_large_directories() { async fn stat_reports_kind_and_size() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("s.bin"); - exchange(&[ - json!({"op":"fs_write","path":path.to_str().unwrap()}).to_string(), - json!({"op":"data","data":b64(&[0u8; 10])}).to_string(), - json!({"op":"data_end"}).to_string(), - ]) + fs_write( + json!({"op":"fs_write","path":path.to_str().unwrap()}), + &[0u8; 10], + ) .await; let st = exchange(&[json!({"op":"fs_stat","path":path.to_str().unwrap()}).to_string()]).await; @@ -101,11 +105,10 @@ async fn stat_reports_kind_and_size() { async fn stat_mode_excludes_type_bits() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("m.txt"); - exchange(&[ - json!({"op":"fs_write","path":path.to_str().unwrap(),"mode":0o640}).to_string(), - json!({"op":"data","data":b64(b"x")}).to_string(), - json!({"op":"data_end"}).to_string(), - ]) + fs_write( + json!({"op":"fs_write","path":path.to_str().unwrap(),"mode":0o640}), + b"x", + ) .await; let st = exchange(&[json!({"op":"fs_stat","path":path.to_str().unwrap()}).to_string()]).await; assert_eq!( @@ -143,9 +146,7 @@ async fn mkdir_rename_rm_lifecycle() { async fn read_of_a_non_regular_file_is_bad_request() { let dir = tempfile::tempdir().unwrap(); let fifo = dir.path().join("pipe"); - let c_fifo = std::ffi::CString::new(fifo.to_str().unwrap()).unwrap(); - // SAFETY: a valid NUL-terminated path; mkfifo touches nothing else. - assert_eq!(unsafe { libc::mkfifo(c_fifo.as_ptr(), 0o600) }, 0); + nix::unistd::mkfifo(&fifo, Mode::S_IRUSR | Mode::S_IWUSR).unwrap(); for path in [ dir.path().to_str().unwrap(), "/dev/null", @@ -196,11 +197,10 @@ async fn overwrite_preserves_destination_mode() { let path = dir.path().join("script.sh"); std::fs::write(&path, b"old").unwrap(); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); - let f = exchange(&[ - json!({"op":"fs_write","path":path.to_str().unwrap()}).to_string(), - json!({"op":"data","data":b64(b"new content")}).to_string(), - json!({"op":"data_end"}).to_string(), - ]) + let f = fs_write( + json!({"op":"fs_write","path":path.to_str().unwrap()}), + b"new content", + ) .await; assert_eq!(type_of(&f[0]), "done"); let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o7777; @@ -208,6 +208,28 @@ async fn overwrite_preserves_destination_mode() { assert_eq!(std::fs::read(&path).unwrap(), b"new content"); } +#[tokio::test] +#[ignore = "needs root"] +async fn overwrite_keeps_the_destination_owner_and_mode_under_root() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("owned"); + for want in [0o644, 0o4755] { + std::fs::write(&path, b"old").unwrap(); + std::os::unix::fs::chown(&path, Some(1000), Some(1000)).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(want)).unwrap(); + let f = fs_write( + json!({"op":"fs_write","path":path.to_str().unwrap()}), + b"new", + ) + .await; + assert_eq!(type_of(&f[0]), "done", "{f:?}"); + let meta = std::fs::metadata(&path).unwrap(); + assert_eq!((meta.uid(), meta.gid()), (1000, 1000)); + assert_eq!(meta.permissions().mode() & 0o7777, want); + assert_eq!(std::fs::read(&path).unwrap(), b"new"); + } +} + #[tokio::test] async fn rm_plain_file_and_missing_and_nonempty_dir() { let dir = tempfile::tempdir().unwrap(); @@ -234,11 +256,10 @@ async fn rm_plain_file_and_missing_and_nonempty_dir() { async fn failed_overwrite_preserves_original() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("keep.txt"); - exchange(&[ - json!({"op":"fs_write","path":path.to_str().unwrap()}).to_string(), - json!({"op":"data","data":b64(b"ORIGINAL")}).to_string(), - json!({"op":"data_end"}).to_string(), - ]) + fs_write( + json!({"op":"fs_write","path":path.to_str().unwrap()}), + b"ORIGINAL", + ) .await; exchange(&[ json!({"op":"fs_write","path":path.to_str().unwrap()}).to_string(), diff --git a/silkd/tests/git_e2e.rs b/silkd/tests/git_e2e.rs index b65f73826..f9e2fcb94 100644 --- a/silkd/tests/git_e2e.rs +++ b/silkd/tests/git_e2e.rs @@ -179,18 +179,7 @@ async fn status_handles_rename_and_spaces() { #[tokio::test] async fn commit_works_without_preconfigured_identity() { - let dir = tempfile::tempdir().unwrap(); - assert!( - std::process::Command::new("git") - .arg("-C") - .arg(dir.path()) - .args(["-c", "init.defaultBranch=main", "init", "-q"]) - .env_remove("GIT_AUTHOR_NAME") - .env_remove("GIT_COMMITTER_NAME") - .status() - .unwrap() - .success() - ); + let dir = init_repo(); std::fs::write(dir.path().join("f"), "x").unwrap(); let p = dir.path().to_str().unwrap(); let add = exchange(&[json!({"op":"git_add","path":p,"files":["f"]}).to_string()]).await; @@ -205,13 +194,22 @@ async fn commit_works_without_preconfigured_identity() { "git_commit_result", "commit failed: {commit:?}" ); + let log = std::process::Command::new("git") + .arg("-C") + .arg(dir.path()) + .args(["log", "-1", "--format=%cn <%ce>"]) + .output() + .unwrap(); + assert_eq!( + String::from_utf8_lossy(&log.stdout).trim(), + "Dev ", + "the committer must come from the author, not the host's git config" + ); } #[tokio::test] +#[ignore = "needs root"] async fn git_runs_as_the_repo_owner_under_root() { - if !nix::unistd::geteuid().is_root() { - return; - } let repo = init_repo(); let p = repo.path().to_str().unwrap(); std::fs::write(repo.path().join("a.txt"), "hello\n").unwrap(); diff --git a/silkd/tests/lsp_e2e.rs b/silkd/tests/lsp_e2e.rs index e45184e4a..199e1f995 100644 --- a/silkd/tests/lsp_e2e.rs +++ b/silkd/tests/lsp_e2e.rs @@ -83,18 +83,25 @@ async fn lsp_start_missing_manifest_is_not_found() { #[tokio::test] async fn lsp_start_language_name_cannot_escape() { let _env_lock = ENV_LOCK.lock().await; - for bad in ["../etc/passwd", "a/b", ".."] { + let (env, state, server_id) = started_server("real", "#!/bin/sh\nsleep 60\n").await; + let base = env.path().file_name().unwrap().to_str().unwrap(); + std::fs::create_dir(env.path().join("sub")).unwrap(); + std::fs::copy(env.path().join("real"), env.path().join("sub/real")).unwrap(); + for bad in [format!("../{base}/real"), "sub/real".to_string()] { let frames = one( - &Arc::new(State::new()), + &state, &json!({"op":"lsp_start","language":bad}).to_string(), ) .await; - assert_eq!( - type_of(frames.last().unwrap()), - "error", - "{bad} should be rejected" - ); + let last = frames.last().unwrap(); + assert_eq!(type_of(last), "error", "{bad} should be rejected"); + assert_eq!(last["kind"], "not_found", "{bad}"); } + one( + &state, + &json!({"op":"lsp_stop","server_id":server_id}).to_string(), + ) + .await; } #[tokio::test] diff --git a/silkd/tests/session_e2e.rs b/silkd/tests/session_e2e.rs index b2969826d..b601c1f61 100644 --- a/silkd/tests/session_e2e.rs +++ b/silkd/tests/session_e2e.rs @@ -227,10 +227,6 @@ async fn session_argv_with_metacharacters_is_one_literal_token() { evil, "argv metacharacters were interpreted instead of passed literally: {r:?}" ); - assert!( - !stdout_body(&r).contains("INJECTED\n") && !stdout_body(&r).to_lowercase().contains("uid="), - "command injection through argv: {r:?}" - ); } #[tokio::test]