From 6e80f4decd7d7d78ec6bd770d52105f4bd5db15a Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Wed, 5 Aug 2026 13:38:36 +0800 Subject: [PATCH 1/4] Add s3 storage --- .mise/config.toml | 12 ++ CLAUDE.md | 20 +-- Cargo.lock | 98 ++++++++++- Cargo.toml | 8 + .../rules/doc-summary-ends-with-period.yaml | 1 + config/conftest/policy/mise/mise.rego | 5 + config/lychee.toml | 7 +- config/semgrep/module-description-length.yaml | 8 +- .../mise-cargo-backend-allowlist.schema.json | 1 + services/storage/Cargo.toml | 6 + services/storage/src/lib.rs | 127 ++++++++++++-- services/storage/src/routes.rs | 143 ++++++++++------ services/storage/src/tty_image.rs | 11 +- services/storage/tests/put.rs | 23 +-- services/storage/tests/s3_backend.rs | 157 ++++++++++++++++++ services/ws-server/src/main.rs | 2 - services/ws-server/tests/configure_app.rs | 2 +- services/ws-test-server/src/lib.rs | 3 +- .../src/deployment_types/docker_compose.rs | 4 +- .../facility-security-scenario/compose.yaml | 2 +- 20 files changed, 531 insertions(+), 109 deletions(-) create mode 100644 services/storage/tests/s3_backend.rs diff --git a/.mise/config.toml b/.mise/config.toml index 29ab7f8e..cb1a9eb6 100644 --- a/.mise/config.toml +++ b/.mise/config.toml @@ -97,6 +97,12 @@ ripgrep = "latest" "github:benhoyt/goawk" = "latest" "github:caldempsey/parfit" = "latest" "github:grok-rs/waitup" = "latest" +# The macos/x64 half of rustfs, the one platform upstream ships no prebuilt for. +# A git spec rather than a bare `cargo:rustfs`: the crates.io crate is a stale 0.0.2 placeholder while real +# releases are 1.0.0-beta.x on GitHub only, so installing the crate would give this platform different software +# than every other one. `tag:` builds the exact source the prebuilt is cut from, keeping them aligned -- bump it +# with the `rustfs` entry. A source build is acceptable here because macos/x64 is second-tier. +"cargo:rustfs/rustfs" = { version = "tag:1.0.0-beta.12", os = ["macos/x64"] } # ONNX Runtime prebuilt -- ort-sys 2.0.0-rc.10 binds to ORT 1.22.x. "cargo:ryl" = { version = "latest", os = ["macos/x64"] } "github:microsoft/onnxruntime" = "1.22.0" @@ -139,6 +145,12 @@ protoc = "latest" # exact `3.x.y` dir. Bump in lockstep with py3_unix/py3_win/pylib. python = "3.13.14" rclone = "latest" +# rustfs is the S3-compatible server the et-storage-service S3-backend test runs against. +# Version-pinned rather than `latest` because upstream has published only prereleases so far, which mise's +# version list filters out -- `latest` resolves to nothing. Upstream ships prebuilts for linux (x64+arm64, +# gnu+musl), macos/arm64 and windows/x64 but NOT macos/x64, so that one platform builds from source via the +# cargo entry below. Bump both in lockstep. +rustfs = { version = "1.0.0-beta.12", os = ["linux", "macos/arm64", "windows"] } typos = "latest" uv = "0.11.8" wasm-tools = "latest" diff --git a/CLAUDE.md b/CLAUDE.md index dedaf05d..ccdafdbe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -126,7 +126,7 @@ languages this repo uses: - **Dockerfile `RUN` block**: switch to BuildKit's HEREDOC form (`RUN bash <<'EOF'` ... `EOF`) -- each shell command sits on its own line with no continuation needed. Three rules, all enforced by - `config/conftest/policy/dockerfile.rego` + the matching semgrep rule + `config/conftest/policy/dockerfile/dockerfile.rego` + the matching semgrep rule under `config/semgrep/`: (1) interpreter is **`bash`, placed BEFORE the `<`. A hit in either tier means install-action will fetch a prebuilt; absence in both means CI would pay a source-build cost. -Even a manifest hit isn't a guarantee -- install-action's resolver is strict about the binary names it expects to -find inside the prebuilt archive, and upstream renames break it silently. Concrete cautionary tale: `aube` IS in -install-action's manifest, but the manifest expects an `aubr` binary -(`When resolving aube bin aubr is not found. This binary is not optional so it must be included in the archive`), -which recent aube releases don't ship. install-action then falls through to a real `cargo install` source build, -which then flakes on crates.io. The mise-managed `setup-aube` task (npm-backed, `continue-on-error`) was the -reliable path here; reach for install-action only when the prebuilt actually exists for the target triple _and_ -the binary names still match. +Even a manifest hit isn't a guarantee -- install-action's resolver insists on the exact binary names its manifest +expects to find inside the prebuilt archive, so an upstream rename makes it fall through to a real `cargo install` +source build (which then flakes on crates.io) with no signal beyond a `bin is not found` line. Reach for +install-action only when the prebuilt actually exists for the target triple _and_ the binary names still match. ## Linting @@ -1087,7 +1083,7 @@ first, and only bring the question to the user if neither is workable -- don't a ## Clippy lints **Never weaken or disable a lint to make code pass -- not the workspace lint config (`[workspace.lints.*]`, -`.clippy.toml` thresholds, the ast-grep / taplo rules) -- without explicit operator permission.** Setting a +`config/clippy.toml` thresholds, the ast-grep / taplo rules) -- without explicit operator permission.** Setting a denied lint to `allow` or raising a threshold is a project-policy change, not a fix. If a lint is in the way, fix the code (or justify it with a scoped `#[expect(..., reason = "...")]`); if you believe the lint itself is wrong, stop and ask. diff --git a/Cargo.lock b/Cargo.lock index 270023a4..2ed01e58 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1926,6 +1926,16 @@ version = "0.134.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f6977c2a71ab1e0d1e62f966b411a498aa04c4dce47d93d52f8a360a06058922" +[[package]] +name = "crc-fast" +version = "1.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e75b2483e97a5a7da73ac68a05b629f9c53cff58d8ed1c77866079e18b00dba5" +dependencies = [ + "digest 0.10.7", + "spin 0.10.1", +] + [[package]] name = "crc32fast" version = "1.5.0" @@ -2928,7 +2938,7 @@ dependencies = [ "hkdf", "hmac", "k256", - "md-5", + "md-5 0.10.6", "md4", "num-bigint", "num-bigint-dig", @@ -4295,19 +4305,24 @@ dependencies = [ "actix-rt", "actix-web", "actix-web-thiserror", + "command-error", "edge-toolkit", + "et-test-helpers", "fs-err", "futures-util", "image", "log", + "object_store", "serde", "serde-inline-default", "serde_default", "serde_json", + "temp-env", "tempfile", "thiserror 2.0.18", "tokio", "tracing", + "url", "utoipa", ] @@ -6329,6 +6344,15 @@ dependencies = [ "either", ] +[[package]] +name = "itertools" +version = "0.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b4baf93f58d4425749ca49a51c50ebab072c5df6994d08fed93541c331481dc" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.18" @@ -6588,7 +6612,7 @@ version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe" dependencies = [ - "spin", + "spin 0.9.9", ] [[package]] @@ -6860,6 +6884,16 @@ dependencies = [ "digest 0.10.7", ] +[[package]] +name = "md-5" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "69b6441f590336821bb897fb28fc622898ccceb1d6cea3fde5ea86b090c4de98" +dependencies = [ + "cfg-if", + "digest 0.11.3", +] + [[package]] name = "md4" version = "0.10.2" @@ -7415,6 +7449,48 @@ dependencies = [ "memchr", ] +[[package]] +name = "object_store" +version = "0.14.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d354792e39fa5f0009e47623cf8b15b099bf9a652fa55c6f817fe28ac84fea50" +dependencies = [ + "async-trait", + "aws-lc-rs", + "base64 0.22.1", + "bytes", + "chrono", + "crc-fast", + "form_urlencoded", + "futures-channel", + "futures-core", + "futures-util", + "http 1.4.2", + "http-body-util", + "humantime", + "hyper", + "itertools 0.15.0", + "md-5 0.11.0", + "nix 0.31.3", + "parking_lot", + "percent-encoding", + "quick-xml", + "rand 0.10.2", + "reqwest 0.13.4", + "rustls-pki-types", + "serde", + "serde_json", + "serde_urlencoded", + "thiserror 2.0.18", + "tokio", + "tracing", + "url", + "walkdir", + "wasm-bindgen-futures", + "web-time", + "windows-sys 0.61.2", +] + [[package]] name = "ocb3" version = "0.1.0" @@ -8516,6 +8592,16 @@ version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a993555f31e5a609f617c12db6250dedcac1b0a85076912c436e6fc9b2c8e6a3" +[[package]] +name = "quick-xml" +version = "0.41.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e660451e55124f798a69a5af3f49ccfbefbd41910eefd25caf2393e1f3473ec1" +dependencies = [ + "memchr", + "serde", +] + [[package]] name = "quinn" version = "0.11.11" @@ -8952,6 +9038,7 @@ dependencies = [ "futures-channel", "futures-core", "futures-util", + "h2 0.4.15", "http 1.4.2", "http-body", "http-body-util", @@ -10063,6 +10150,12 @@ version = "0.9.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3763264f6b73151db08c50ff20d7d8a0b8796e021cdea7ceedad07b80155fa0e" +[[package]] +name = "spin" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "023a211cb3138dbc438680b32560ad89f699977624c9f8dbb95a47d5b4c07dd3" + [[package]] name = "spirv" version = "0.4.0+sdk-1.4.341.0" @@ -10729,6 +10822,7 @@ version = "0.3.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "96374855068f47402c3121c6eed88d29cb1de8f3ab27090e273e420bdabcf050" dependencies = [ + "futures", "parking_lot", ] diff --git a/Cargo.toml b/Cargo.toml index 2a1d9181..14da0602 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -106,6 +106,11 @@ local-ip-address = "0.6" log = "0.4" minicov = "0.3" onnx-extractor = "0.3" +# Storage backend abstraction: local disk by default, any object_store backend via a URL. +# `aws` pulls the S3 client (which also covers S3-compatible servers such as the rustfs mise tool the backend +# test runs against). Only `aws` is enabled -- adding `gcp`/`azure`/`http` is a one-word change if a consumer +# ever needs them, and each drags its own SDK surface, so they stay off until asked for. +object_store = { version = "0.14", default-features = false, features = ["aws", "fs"] } openapiv3 = "2" opentelemetry = "0.31" opentelemetry-appender-tracing = "0.31" @@ -182,6 +187,9 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } tracing-wasm = "0.2" tree-sitter = "0.25" tree-sitter-zig = "1" +# Already resolved transitively (object_store, reqwest, ...), so this adds no new crate or license to review. +# object_store's parse_url_opts takes a `&Url` and does not re-export the type, so a backend-URL config needs it. +url = "2" utoipa = { version = "5", features = ["actix_extras", "yaml"] } uuid = { version = "1", features = ["serde", "v4", "v7"] } wasm-bindgen = "0.2" diff --git a/config/ast-grep/rules/doc-summary-ends-with-period.yaml b/config/ast-grep/rules/doc-summary-ends-with-period.yaml index c9673deb..16b21520 100644 --- a/config/ast-grep/rules/doc-summary-ends-with-period.yaml +++ b/config/ast-grep/rules/doc-summary-ends-with-period.yaml @@ -38,6 +38,7 @@ files: - services/storage/src/tty_image.rs - services/storage/tests/image_logging.rs - services/storage/tests/put.rs + - services/storage/tests/s3_backend.rs - services/websockify/src/lib.rs - services/websockify/tests/relay.rs - services/ws-modules/except1/src/lib.rs diff --git a/config/conftest/policy/mise/mise.rego b/config/conftest/policy/mise/mise.rego index 94a065a6..8e862192 100644 --- a/config/conftest/policy/mise/mise.rego +++ b/config/conftest/policy/mise/mise.rego @@ -163,6 +163,11 @@ allowed_os_scoped_tool := { # windows_exporter is a Windows-only host/GPU Prometheus exporter, so the o2-winmetrics task scopes it to Windows. "github:prometheus-community/windows_exporter", "cargo:findutils", + # rustfs is the S3 server the storage backend test runs against. + # Upstream ships no macos/x64 prebuilt, so that one platform builds from the same git tag via cargo while the + # others take the github release. Between the two entries every platform is covered. + "rustfs", + "cargo:rustfs/rustfs", "cargo:ryl", # http:et-rp is os-scoped to only those platforms whose tarball is already in the rp-v release. # Add a platform by dispatching the upstream-cache.yaml workflow on that host. diff --git a/config/lychee.toml b/config/lychee.toml index c3b52399..b24279fc 100644 --- a/config/lychee.toml +++ b/config/lychee.toml @@ -36,4 +36,9 @@ exclude_path = ["data", "generated", "node_modules", "target"] # [404] https://github.com/edge-toolkit/core/actions/runs/28698136445/job/85111320237 (at 447:1) # from CLAUDE.md's attestation-flake note on commit e401831a98197068f978ef760134caff882e2615 while the same # URL returned 200 to a local curl minutes later. Checking them is all noise, no signal. -exclude = ['\{|%7B', '^https://github\.com/[^/]+/[^/]+/actions/runs/', '^https?://host[:/]'] +# +# The `file:` entry is the container-internal storage mount the compose generator emits as a `STORAGE_URL` +# value; it names a path inside the ws-server image, so it is never resolvable from the host doing the check. +# Scoped to that exact URL rather than the whole `file:` scheme, because relative links in markdown resolve to +# `file:` URIs too and those are real links worth checking. +exclude = ['\{|%7B', '^file:///app/storage$', '^https://github\.com/[^/]+/[^/]+/actions/runs/', '^https?://host[:/]'] diff --git a/config/semgrep/module-description-length.yaml b/config/semgrep/module-description-length.yaml index 4e5e2228..92068a16 100644 --- a/config/semgrep/module-description-length.yaml +++ b/config/semgrep/module-description-length.yaml @@ -3,10 +3,10 @@ rules: languages: [generic] paths: include: - - "services/ws-modules/*/Cargo.toml" - - "services/ws-modules/*/pyproject.toml" - - "services/ws-modules/*/build.zig.zon" - - "services/ws-modules/*/pkg/package.json" + - "/services/ws-modules/*/Cargo.toml" + - "/services/ws-modules/*/pyproject.toml" + - "/services/ws-modules/*/build.zig.zon" + - "/services/ws-modules/*/pkg/package.json" # Enforce ws-module description length at the source manifests, not by trimming labels in the browser. # A ws-module description is the module-dropdown label on the ws-server index page. Two regex alternatives # cover the syntaxes the description travels through: `description = "..."` (Cargo.toml / pyproject.toml, diff --git a/config/taplo/mise-cargo-backend-allowlist.schema.json b/config/taplo/mise-cargo-backend-allowlist.schema.json index ab1ac332..21696781 100644 --- a/config/taplo/mise-cargo-backend-allowlist.schema.json +++ b/config/taplo/mise-cargo-backend-allowlist.schema.json @@ -16,6 +16,7 @@ "cargo:dart-typegen", "cargo:findutils", "cargo:open", + "cargo:rustfs/rustfs", "cargo:ryl", "cargo:wasm-opt" ] diff --git a/services/storage/Cargo.toml b/services/storage/Cargo.toml index 18ceb414..9c54da5f 100644 --- a/services/storage/Cargo.toml +++ b/services/storage/Cargo.toml @@ -21,6 +21,7 @@ fs-err.workspace = true futures-util.workspace = true image.workspace = true log.workspace = true +object_store.workspace = true serde.workspace = true serde-inline-default.workspace = true serde_default.workspace = true @@ -28,10 +29,15 @@ serde_json.workspace = true thiserror.workspace = true tokio = { workspace = true, features = ["full"] } tracing.workspace = true +url.workspace = true utoipa = { workspace = true, optional = true } [dev-dependencies] actix-rt.workspace = true +command-error.workspace = true +et-test-helpers.workspace = true +object_store.workspace = true +temp-env = { workspace = true, features = ["async_closure"] } tempfile.workspace = true [lints] diff --git a/services/storage/src/lib.rs b/services/storage/src/lib.rs index c7d6627a..c4ba7501 100644 --- a/services/storage/src/lib.rs +++ b/services/storage/src/lib.rs @@ -1,9 +1,18 @@ +//! Agent file storage, backed by any `object_store` backend. +//! +//! The wire protocol is unchanged (`PUT`/`GET /storage/{agent_id}/{filename}`); only the storage layer beneath +//! it is pluggable. [`StorageConfig::url`] selects the backend and defaults to a `file://` URL under +//! [`default_storage_folder`], so nothing needs configuring for research use; an `object_store` URL such as +//! `s3://bucket` points at a remote instead. Objects are addressed as `/` under whichever +//! store is in use, so the local-disk layout is the same as before. + use std::path::PathBuf; -use std::sync::PoisonError; +use std::sync::{Arc, PoisonError}; -use actix_files::Files; use actix_web::web; use actix_web_thiserror::ResponseError; +use object_store::ObjectStore; +use object_store::local::LocalFileSystem; use serde::Deserialize; use serde_default::DefaultFromSerde; use thiserror::Error; @@ -11,7 +20,7 @@ use thiserror::Error; pub mod routes; mod tty_image; -pub use self::routes::put_file; +pub use self::routes::{get_file, put_file}; /// Default storage directory. #[must_use] @@ -20,21 +29,58 @@ pub fn default_storage_folder() -> PathBuf { project_root.join("services/ws-server/storage") } +/// `file://` URL for [`default_storage_folder`], the backend used when nothing is configured. +/// +/// This is the default baked into [`StorageConfig::url`], so the local-disk backend is expressed the same way +/// as any other -- as a URL -- rather than being inferred later from an absent field. +#[must_use] +pub fn default_storage_url() -> String { + file_url(&default_storage_folder()) +} + +/// Render `path` as a `file://` URL. +/// +/// `Url::from_directory_path` only fails for a non-absolute path, and every caller here passes an absolute one +/// (the project root, or a tempdir). The fallback keeps this infallible rather than forcing a `Result` through +/// serde's default hook, and is not reachable for an absolute input on any platform. +#[must_use] +pub fn file_url(path: &std::path::Path) -> String { + url::Url::from_directory_path(path).map_or_else(|()| format!("file://{}", path.display()), String::from) +} + /// Storage config. #[derive(Clone, Debug, DefaultFromSerde, Deserialize)] #[non_exhaustive] pub struct StorageConfig { - #[serde(default = "default_storage_folder")] - pub path: PathBuf, + /// `object_store` backend URL, defaulting to local disk under [`default_storage_folder`]. + /// + /// One field rather than a path plus an optional override: the backend has exactly one source of truth, and + /// the default is declared here rather than reconstructed inside [`build_store`]. Anything `object_store` + /// recognises for a compiled-in backend works -- a `file:` URL naming an absolute directory, `s3://bucket` + /// (including S3-compatible servers), `memory://`. Credentials, endpoint and addressing style come from each + /// backend's own standard environment variables rather than config keys of our own, so an operator configures + /// a store exactly as they would for any other client of it. + #[serde(default = "default_storage_url")] + pub url: String, } impl StorageConfig { + /// Build a config for an explicit backend URL. #[must_use] - pub const fn new(path: PathBuf) -> Self { - Self { path } + pub const fn new(url: String) -> Self { + Self { url } + } + + /// Build a config for a local directory, for callers holding a path rather than a URL. + #[must_use] + pub fn local(path: &std::path::Path) -> Self { + Self { url: file_url(path) } } } +/// Shared handle to the configured backend, held in actix app data and by the routes. +pub type SharedStore = Arc; + #[derive(Debug, Error, ResponseError)] #[non_exhaustive] pub enum StorageError { @@ -46,6 +92,10 @@ pub enum StorageError { #[response(status = 404, reason = "NOT_FOUND")] AgentNotFound, + #[error("no such object")] + #[response(status = 404, reason = "NOT_FOUND")] + ObjectNotFound, + #[error("agent registry lock poisoned")] AgentRegistryPoisoned, @@ -54,6 +104,12 @@ pub enum StorageError { #[error(transparent)] Payload(#[from] actix_web::error::PayloadError), + + #[error(transparent)] + Store(#[from] object_store::Error), + + #[error(transparent)] + StoreUrl(#[from] url::ParseError), } // `PoisonError` is generic over the guard type; a generic `From` impl @@ -64,18 +120,59 @@ impl From> for StorageError { } } -/// Register `PUT /storage/{agent_id}/{filename}` and `GET /storage/...` (static file serving). +/// Build the backend described by `config`. +/// +/// Remote backends are configured from the process environment, so credentials can be supplied as separate +/// environment variables (a mounted Kubernetes secret, say) instead of being embedded in the URL. `object_store` +/// errors clearly if the URL names a backend whose feature is not compiled in -- currently only `aws`, plus the +/// always-present `file` and `memory`. +pub fn build_store(config: &StorageConfig) -> Result { + let parsed = url::Url::parse(&config.url)?; + + // `file` is the one scheme handled here rather than by object_store's own dispatch. + // `parse_url_opts` maps it to `LocalFileSystem::new()` -- no prefix -- which would resolve every + // `/` key against the filesystem root instead of the configured directory. Prefixing the + // store is what makes keys relative, matching how every remote backend already behaves, and the directory + // has to exist first for `new_with_prefix` to accept it (which also preserves first-run behaviour). + if parsed.scheme() == "file" { + let root = parsed.to_file_path().unwrap_or_else(|()| PathBuf::from(parsed.path())); + fs_err::create_dir_all(&root)?; + return Ok(Arc::new(LocalFileSystem::new_with_prefix(&root)?)); + } + + // Every other scheme gets the process environment as options, which is object_store's documented pattern + // (see `parse_url_opts`) and the reason each backend can be configured -- credentials included -- from + // separate environment variables rather than from secrets embedded in the URL. `builder_opts!` lowercases + // each key and silently skips ones the backend does not recognise, so the whole environment can be handed + // over: `AWS_SECRET_ACCESS_KEY`, `GOOGLE_SERVICE_ACCOUNT_KEY`, `AZURE_STORAGE_ACCOUNT_KEY` and friends each + // land on their own backend, which is what makes a Kubernetes secret mounted as an env var work here. + // Doing this per-scheme with each builder's own `from_env` would have given S3 that treatment and quietly + // denied it to the others. + let (store, _prefix) = object_store::parse_url_opts(&parsed, std::env::vars())?; + Ok(Arc::from(store)) +} + +/// Register `PUT /storage/{agent_id}/{filename}` and `GET /storage/{agent_id}/{filename}`. +/// +/// # Panics +/// +/// Panics if `config` describes a backend that cannot be constructed (an unparsable URL, a scheme whose +/// backend is not compiled in, or an unusable local path). That is a misconfigured deployment rather than a +/// runtime condition, so it fails at startup instead of turning every later request into a 500. +#[expect( + clippy::panic, + reason = "an unbuildable backend is a misconfigured deployment; fail at startup, not on every request" +)] pub fn configure(cfg: &mut web::ServiceConfig, config: &StorageConfig) where S: Clone + Send + 'static, { - let storage_dir = config.path.clone(); + let store = match build_store(config) { + Ok(store) => store, + Err(error) => panic!("storage backend is misconfigured: {error}"), + }; let _configured = cfg + .app_data(web::Data::::from(store)) .route("/storage/{agent_id}/{filename}", web::put().to(put_file::)) - .service( - Files::new("/storage", storage_dir) - .show_files_listing() - .use_etag(true) - .use_last_modified(true), - ); + .route("/storage/{agent_id}/{filename}", web::get().to(get_file)); } diff --git a/services/storage/src/routes.rs b/services/storage/src/routes.rs index 067d7938..146db52f 100644 --- a/services/storage/src/routes.rs +++ b/services/storage/src/routes.rs @@ -1,10 +1,12 @@ //! Storage HTTP routes carrying `#[utoipa::path]` annotations. //! -//! `put_file` is the live PUT handler; `get_file` is a fake stub whose -//! only role is to host the `#[utoipa::path]` annotation for the GET -//! route (actually served by an `actix_files::Files` mount registered -//! in [`crate::configure`]). `BinaryBlob` is the phantom request-body -//! schema both routes reference. +//! Both handlers are live and go through the configured `object_store` +//! backend, so the same code serves local disk and any remote store. +//! `get_file` used to be a stub hosting only the `#[utoipa::path]` +//! annotation, with an `actix_files::Files` mount doing the real work -- +//! that mount could only ever read local disk, so it is gone. +//! `BinaryBlob` is the phantom request-body schema both routes +//! reference. //! //! The file-level `#![expect(clippy::exhaustive_structs)]` (active //! under `openapi-spec`) is scoped here because utoipa's derives emit @@ -24,9 +26,35 @@ use std::path::PathBuf; use actix_web::{HttpRequest, HttpResponse, web}; use edge_toolkit::ws_server::AgentRegistry; use futures_util::StreamExt as _; +use object_store::{ObjectStore, ObjectStoreExt as _, PutPayload}; use tracing::{info, warn}; -use crate::{StorageConfig, StorageError}; +use crate::StorageError; + +/// Object key for `filename` inside `agent_id`'s bucket. +/// +/// Every backend addresses objects the same way, so this is the one place the `/` layout is +/// spelled out -- it is also what keeps a local-disk store laid out exactly as the previous fs implementation. +fn object_path(agent_id: &str, filename: &std::path::Path) -> object_store::path::Path { + object_store::path::Path::from(format!("{agent_id}/{}", filename.display())) +} + +/// Extract and validate the `{agent_id}`/`{filename}` pair from the request path. +/// +/// The filename must be a single path component: a nested or absolute path would let a caller address objects +/// outside its own bucket. +fn agent_and_filename(req: &HttpRequest) -> Result<(String, PathBuf), StorageError> { + let agent_id = req.match_info().query("agent_id").to_string(); + let filename = req + .match_info() + .query("filename") + .parse::() + .ok() + .filter(|filename| filename.components().count() == 1) + .ok_or(StorageError::InvalidFilename)?; + + Ok((agent_id, filename)) +} /// Phantom type used to label binary request/response bodies as `string`/`binary`. /// @@ -72,51 +100,44 @@ pub async fn put_file( req: HttpRequest, mut payload: web::Payload, registry: web::Data>, - config: web::Data, + store: web::Data, ) -> Result where S: Clone + Send + 'static, { - let agent_id = req.match_info().query("agent_id").to_string(); - let filename = req - .match_info() - .query("filename") - .parse::() - .ok() - .filter(|filename| filename.components().count() == 1) - .ok_or(StorageError::InvalidFilename)?; + let (agent_id, filename) = agent_and_filename(&req)?; if !registry.agents.lock()?.contains_key(&agent_id) { return Err(StorageError::AgentNotFound); } - let storage_dir = &config.path; - let agent_dir = storage_dir.join(&agent_id); - fs_err::create_dir_all(&agent_dir)?; - - let path = agent_dir.join(&filename); - info!("Agent {} storing file: {:?}", agent_id, path); - - let mut file = tokio::fs::File::create(&path).await?; - let mut bytes_written: u64 = 0; + // The body is buffered rather than streamed to the store. + // `ObjectStore::put` takes a whole payload, and the alternative (`put_multipart`) buys streaming at the + // cost of chunk bookkeeping that these payloads -- module output and captured frames -- do not need. The + // buffer also gives the tty preview below the bytes for free, where the previous fs implementation could + // rely on re-reading the file it had just written. + let mut body: Vec = Vec::new(); while let Some(chunk) = payload.next().await { - let chunk = chunk?; - let copied = tokio::io::copy(&mut chunk.as_ref(), &mut file).await?; - bytes_written = bytes_written.saturating_add(copied); + body.extend_from_slice(&chunk?); } + let bytes_written = body.len(); + + let path = object_path(&agent_id, &filename); + info!("Agent {} storing file: {}", agent_id, path); + let _put_result = store.put(&path, PutPayload::from(body.clone())).await?; // This handler is the only path a file reaches storage through, so it is where every stored file can be // watched exactly once with an accurate byte count and zero extra I/O -- a separate filesystem watcher - // would duplicate that work and risk observing a write mid-flight. + // would duplicate that work, risk observing a write mid-flight, and could not see a remote backend at all. if is_image_filename(&filename) { - info!("Agent {} stored image {:?} ({} bytes)", agent_id, path, bytes_written); - show_image_on_tty(&path); + info!("Agent {} stored image {} ({} bytes)", agent_id, path, bytes_written); + show_image_on_tty(&body); } Ok(HttpResponse::Ok().finish()) } -/// Render a thumbnail of the image at `path` directly to stdout. +/// Render a thumbnail of the just-stored image bytes directly to stdout. /// /// This lets the operator actually *see* what was stored, rather than just its filename and byte count. It /// bypasses `tracing` entirely and writes straight to the terminal: the escape sequences (ANSI @@ -124,13 +145,16 @@ where /// data, and would otherwise get shipped to the OTLP log exporter as log-record noise. Decode/render /// failures (a corrupt upload, a non-terminal stdout) only cost tty visibility, not the request, so they're /// reported via `warn!` rather than via `?`. +/// +/// Takes bytes rather than a path because the object may never exist on the local filesystem -- with a remote +/// backend there is nothing to re-read. #[expect( clippy::single_call_fn, reason = "distinct step of put_file; kept separate for readability and testing" )] -fn show_image_on_tty(path: &std::path::Path) { - if let Err(error) = crate::tty_image::render(path) { - warn!("failed to render stored image {} to tty: {error}", path.display()); +fn show_image_on_tty(bytes: &[u8]) { + if let Err(error) = crate::tty_image::render_bytes(bytes) { + warn!("failed to render stored image to tty: {error}"); } } @@ -148,25 +172,38 @@ pub fn is_image_filename(filename: &std::path::Path) -> bool { } /// Download a file previously written to the named agent's storage bucket. -#[cfg(feature = "openapi-spec")] -#[utoipa::path( - get, - path = "/storage/{agent_id}/{filename}", - tag = "storage", - params( - ("agent_id" = String, Path, description = "Agent identifier"), - ("filename" = String, Path, description = "Stored filename") - ), - responses( - (status = 200, description = "Stored file contents", content_type = "application/octet-stream"), - (status = 404, description = "No such file") +#[cfg_attr( + feature = "openapi-spec", + utoipa::path( + get, + path = "/storage/{agent_id}/{filename}", + tag = "storage", + params( + ("agent_id" = String, Path, description = "Agent identifier"), + ("filename" = String, Path, description = "Stored filename") + ), + responses( + (status = 200, description = "Stored file contents", content_type = "application/octet-stream"), + (status = 404, description = "No such file") + ) ) )] -#[must_use] -pub fn get_file() -> HttpResponse { - // Fake handler -- the GET route is actually served by the - // `actix_files::Files` mount registered in `crate::configure`; this - // stub exists only to host the `#[utoipa::path]` annotation so - // `et-int-gen` can include the GET route in `generated/specs/rest.yaml`. - HttpResponse::NotImplemented().finish() +#[expect( + clippy::future_not_send, + reason = "actix-web HttpRequest is !Send by design; handler runs on actix's single-threaded runtime" +)] +pub async fn get_file(req: HttpRequest, store: web::Data) -> Result { + let (agent_id, filename) = agent_and_filename(&req)?; + let path = object_path(&agent_id, &filename); + + // A missing object is a 404, not a 500, and it is the one store error worth distinguishing. Matched rather + // than mapped because `.map_err` is banned outside this workspace's designated error-wrapper modules. + let object = match store.get(&path).await { + Ok(object) => object, + Err(object_store::Error::NotFound { .. }) => return Err(StorageError::ObjectNotFound), + Err(error) => return Err(error.into()), + }; + let body = object.bytes().await?; + + Ok(HttpResponse::Ok().content_type("application/octet-stream").body(body)) } diff --git a/services/storage/src/tty_image.rs b/services/storage/src/tty_image.rs index ea705fd6..c65ce66b 100644 --- a/services/storage/src/tty_image.rs +++ b/services/storage/src/tty_image.rs @@ -14,12 +14,15 @@ use std::fmt::Write as _; use std::io::Write as _; -use std::path::Path; /// Terminal columns the rendered thumbnail is resized to fit within. const TARGET_COLUMNS: u32 = 48; -/// Render a thumbnail of the image at `path` directly to stdout using ANSI truecolor half-block art. +/// Render a thumbnail of in-memory image bytes to stdout using ANSI truecolor half-block art. +/// +/// Takes bytes rather than a path because the object may never exist on the local filesystem: with a remote +/// `object_store` backend there is nothing to open, and even locally the storage handler already holds the +/// buffer it just wrote, so decoding from memory avoids a read-back. /// /// Returns the underlying `image` decode error on failure; the caller decides how to report it (this module /// stays IO-boundary-agnostic rather than picking a logging mechanism itself). @@ -27,8 +30,8 @@ const TARGET_COLUMNS: u32 = 48; clippy::single_call_fn, reason = "distinct step of show_image_on_tty; kept separate for readability and testing" )] -pub fn render(path: &Path) -> image::ImageResult<()> { - let source = image::open(path)?; +pub fn render_bytes(bytes: &[u8]) -> image::ImageResult<()> { + let source = image::load_from_memory(bytes)?; if source.width() == 0 || source.height() == 0 { return Ok(()); } diff --git a/services/storage/tests/put.rs b/services/storage/tests/put.rs index 436711b0..f7f2c503 100644 --- a/services/storage/tests/put.rs +++ b/services/storage/tests/put.rs @@ -17,7 +17,7 @@ use actix_web::http::StatusCode; use actix_web::{App, FromRequest as _, test, web}; use edge_toolkit::ws::AgentConnectionState; use edge_toolkit::ws_server::{AgentRecord, AgentRegistry}; -use et_storage_service::{StorageConfig, StorageError, configure, put_file}; +use et_storage_service::{StorageConfig, StorageError, build_store, configure, put_file}; use tempfile::TempDir; /// Build a registry with a single connected agent. @@ -31,7 +31,7 @@ fn registry_with_agent(agent_id: &str) -> AgentRegistry<()> { } fn storage_config(tmp: &TempDir) -> StorageConfig { - StorageConfig::new(tmp.path().to_path_buf()) + StorageConfig::local(tmp.path()) } #[actix_rt::test] @@ -75,7 +75,8 @@ async fn rejects_multi_component_filename_with_400() { let mut payload = DevPayload::None; let payload = web::Payload::from_request(&http_req, &mut payload).await.unwrap(); - let result = put_file::<()>(http_req, payload, web::Data::new(registry), web::Data::new(config)).await; + let store = web::Data::::from(build_store(&config).unwrap()); + let result = put_file::<()>(http_req, payload, web::Data::new(registry), store).await; let err = result.unwrap_err(); assert!(matches!(err, StorageError::InvalidFilename)); @@ -139,14 +140,16 @@ async fn stores_an_image_and_returns_200_even_though_tty_rendering_cannot_succee #[actix_rt::test] async fn surfaces_io_failure_as_500() { - // Point the storage root at a *file* (not a directory). The handler's - // first I/O op is `create_dir_all`, which fails with `NotADirectory` - // when one of the ancestors is a regular file. That propagates through - // `StorageError::Io` and the derived `ResponseError` impl returns 500. + // Block the *agent's* directory with a regular file, so the store's write fails on an ancestor that is not + // a directory. That surfaces as `StorageError::Store` and the derived `ResponseError` impl returns 500. + // + // The storage root itself stays a valid directory on purpose: an unusable root is a misconfigured + // deployment, which `configure` now rejects at startup rather than turning into a per-request 500, so + // pointing the root at a file (what this test used to do) would panic before any request was served. + // Blocking one level down keeps the test on the request path it is actually about. let tmp = tempfile::tempdir().unwrap(); - let blocker = tmp.path().join("blocker"); - fs_err::write(&blocker, b"i am a file, not a directory").unwrap(); - let config = StorageConfig::new(blocker); + fs_err::write(tmp.path().join("agent-1"), b"i am a file, not a directory").unwrap(); + let config = StorageConfig::local(tmp.path()); let registry = registry_with_agent("agent-1"); let app = test::init_service( App::new() diff --git a/services/storage/tests/s3_backend.rs b/services/storage/tests/s3_backend.rs new file mode 100644 index 00000000..b79b7873 --- /dev/null +++ b/services/storage/tests/s3_backend.rs @@ -0,0 +1,157 @@ +//! Round-trips the storage service against a real S3 server to prove the non-default backend works. +//! +//! The default backend is local disk and is covered by `put.rs`; this file exercises the other half of +//! `StorageConfig::url` -- that pointing it at `s3://` makes the same `PUT`/`GET` routes read and write +//! through `object_store`'s S3 client instead, with no change to the wire protocol. +//! +//! The server under test is `rustfs`, a Rust S3 implementation installed as a mise tool. Two things about it +//! shape this test, both established by probing it directly: +//! +//! 1. It does **not** auto-create buckets -- a `PUT` into an unknown bucket is a 404. It does, however, adopt a +//! directory that already exists in its volume, so the bucket is created with `mkdir` before startup rather +//! than by a signed `CreateBucket` call. That keeps the test free of an HTTP client and a `SigV4` signer. +//! 2. Objects are stored erasure-coded as `///xl.meta`, so the assertion that the bytes +//! really landed in S3 checks for that directory rather than a plain file. +//! +//! Credentials reach `build_store` through the conventional `AWS_*` variables, which it forwards to +//! `object_store` as options -- so this also covers the mechanism that lets an operator supply secrets as +//! separate environment variables. They are scoped with `temp_env` so they never leak into another test in the +//! same process. + +#![cfg(test)] + +use std::collections::BTreeMap; +use std::path::Path; +use std::process::{Command, Stdio}; + +use actix_web::http::StatusCode; +use actix_web::{App, test, web}; +use command_error::CommandExt as _; +use edge_toolkit::ws::AgentConnectionState; +use edge_toolkit::ws_server::{AgentRecord, AgentRegistry}; +use et_storage_service::{StorageConfig, configure}; +use et_test_helpers::{ChildGuard, reserve_port, wait_for_port}; + +/// Single bucket holding every agent's objects, keyed `/`. +/// +/// One bucket rather than one per agent keeps the key layout identical to the local-disk backend, so the same +/// `object_path` works unchanged for both. +const BUCKET: &str = "et-storage"; +const AGENT: &str = "agent-s3"; +const FILENAME: &str = "payload.txt"; +const BODY: &[u8] = b"bytes that must survive a round-trip through S3"; + +/// rustfs requires credentials, so the test supplies throwaway ones and hands the same pair to `object_store`. +const ACCESS_KEY: &str = "et-test-access-key"; +const SECRET_KEY: &str = "et-test-secret-key"; + +#[expect( + clippy::single_call_fn, + reason = "mirrors put.rs's helper of the same name; kept for symmetry" +)] +fn registry_with_agent(agent_id: &str) -> AgentRegistry<()> { + let mut agents = BTreeMap::new(); + let _inserted = agents.insert( + agent_id.to_string(), + AgentRecord::new(AgentConnectionState::Connected, None, Some(())), + ); + AgentRegistry::from_agents(agents) +} + +/// Start `rustfs` on `port` serving `volume`, and wait for it to accept connections. +/// +/// Spawned by bare name so the mise-managed binary on `PATH` is used. If it is missing the `expect` below fails +/// loudly -- a missing mise tool is a misconfigured environment, not a reason for this test to quietly pass. +#[expect( + clippy::expect_used, + clippy::single_call_fn, + reason = "distinct setup step, and the expect message names the missing mise tool an unwrap would hide" +)] +fn start_rustfs(volume: &Path, port: u16) -> ChildGuard { + let child = Command::new("rustfs") + .arg("server") + .arg(volume) + .arg("--address") + .arg(format!("127.0.0.1:{port}")) + .arg("--access-key") + .arg(ACCESS_KEY) + .arg("--secret-key") + .arg(SECRET_KEY) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn_checked() + .expect("rustfs must be on PATH (mise tool `rustfs`)") + .into_child(); + let guard = ChildGuard::new(child); + assert!(wait_for_port(port), "rustfs did not start listening on port {port}"); + guard +} + +/// The `AWS_*` pairs `object_store`'s S3 builder consumes, pointing it at the local rustfs. +/// +/// `AWS_ALLOW_HTTP` is required because the endpoint is plain HTTP; addressing stays path-style, which is +/// `object_store`'s default and the only style rustfs serves unless `--server-domains` is set. +#[expect( + clippy::single_call_fn, + reason = "names the env contract under test; kept separate for readability" +)] +fn aws_env(port: u16) -> Vec<(&'static str, Option)> { + vec![ + ("AWS_ENDPOINT", Some(format!("http://127.0.0.1:{port}"))), + ("AWS_ACCESS_KEY_ID", Some(ACCESS_KEY.to_string())), + ("AWS_SECRET_ACCESS_KEY", Some(SECRET_KEY.to_string())), + ("AWS_DEFAULT_REGION", Some("us-east-1".to_string())), + ("AWS_ALLOW_HTTP", Some("true".to_string())), + ] +} + +#[actix_rt::test] +async fn round_trips_put_and_get_through_the_s3_backend() { + let volume = tempfile::tempdir().unwrap(); + // Pre-create the bucket: rustfs adopts an existing volume directory but will not create one on demand. + fs_err::create_dir_all(volume.path().join(BUCKET)).unwrap(); + let port = reserve_port(); + let mut rustfs = start_rustfs(volume.path(), port); + + let config = StorageConfig::new(format!("s3://{BUCKET}")); + + let (put_status, get_status, body) = temp_env::async_with_vars(aws_env(port), async { + let app = test::init_service( + App::new() + .app_data(web::Data::new(registry_with_agent(AGENT))) + .configure(|cfg| configure::<()>(cfg, &config)), + ) + .await; + + let put = test::TestRequest::put() + .uri(&format!("/storage/{AGENT}/{FILENAME}")) + .set_payload(BODY) + .to_request(); + let put_status = test::call_service(&app, put).await.status(); + + let get = test::TestRequest::get() + .uri(&format!("/storage/{AGENT}/{FILENAME}")) + .to_request(); + let get_resp = test::call_service(&app, get).await; + let get_status = get_resp.status(); + let body = test::read_body(get_resp).await; + + (put_status, get_status, body) + }) + .await; + + rustfs.shutdown(); + + assert_eq!(put_status, StatusCode::OK, "PUT through the S3 backend should succeed"); + assert_eq!(get_status, StatusCode::OK, "GET through the S3 backend should succeed"); + assert_eq!(&*body, BODY, "bytes must survive the S3 round-trip unchanged"); + + // Proves the S3 backend actually served the request rather than some local fallback: the object exists + // inside rustfs's own volume, as its erasure-coded object directory. + let stored = volume.path().join(BUCKET).join(AGENT).join(FILENAME); + assert!( + stored.is_dir(), + "expected the object at {} in rustfs's volume", + stored.display() + ); +} diff --git a/services/ws-server/src/main.rs b/services/ws-server/src/main.rs index c6acd350..a4eb9749 100644 --- a/services/ws-server/src/main.rs +++ b/services/ws-server/src/main.rs @@ -97,8 +97,6 @@ async fn main() -> Result<(), Box> { let registry_clone = agent_registry.clone(); let registry_path = args.agent_registry.clone(); - fs_err::create_dir_all(&env.storage.path).unwrap(); - for (name, pkg_dir) in list_modules(&env.modules) { info!("Loading module {name} at {}", pkg_dir.display()); } diff --git a/services/ws-server/tests/configure_app.rs b/services/ws-server/tests/configure_app.rs index 66d25691..12b1ecbb 100644 --- a/services/ws-server/tests/configure_app.rs +++ b/services/ws-server/tests/configure_app.rs @@ -19,7 +19,7 @@ async fn no_content_returns_204() { async fn configure_app_wires_favicon_health_and_modules() { let storage_dir = tempdir().unwrap(); let mut config = Config::default(); - config.storage = StorageConfig::new(storage_dir.path().to_path_buf()); + config.storage = StorageConfig::local(storage_dir.path()); let registry = web::Data::new(WsAgentRegistry::default()); let app = test::init_service(App::new().configure(|cfg| configure_app(cfg, registry, &config))).await; diff --git a/services/ws-test-server/src/lib.rs b/services/ws-test-server/src/lib.rs index 7e820ed3..120cf109 100644 --- a/services/ws-test-server/src/lib.rs +++ b/services/ws-test-server/src/lib.rs @@ -43,9 +43,8 @@ pub fn start() -> TestServer { #[must_use] pub fn start_on(port: u16) -> TestServer { let storage_dir = TempDir::new().unwrap(); - let storage_path = storage_dir.path().to_path_buf(); - let storage_config = StorageConfig::new(storage_path); + let storage_config = StorageConfig::local(storage_dir.path()); let modules_config = ModulesConfig::default(); let addr = format!("127.0.0.1:{port}"); diff --git a/utilities/cli/src/deployment_types/docker_compose.rs b/utilities/cli/src/deployment_types/docker_compose.rs index ccd4664a..406bdce2 100644 --- a/utilities/cli/src/deployment_types/docker_compose.rs +++ b/utilities/cli/src/deployment_types/docker_compose.rs @@ -66,8 +66,8 @@ pub fn generate_docker_compose_deployment(cluster: &ClusterInput, output_dir: &P ComposeValue::Plain("http://127.0.0.1:5080/api/default/v1".to_string()), ), ( - "STORAGE_PATH".to_string(), - ComposeValue::Plain("/app/storage".to_string()), + "STORAGE_URL".to_string(), + ComposeValue::Plain("file:///app/storage".to_string()), ), ], volumes: vec!["ws-server-storage:/app/storage".to_string()], diff --git a/verification/local/output/facility-security-scenario/compose.yaml b/verification/local/output/facility-security-scenario/compose.yaml index 7f0abe55..9206e708 100644 --- a/verification/local/output/facility-security-scenario/compose.yaml +++ b/verification/local/output/facility-security-scenario/compose.yaml @@ -38,7 +38,7 @@ services: OTLP_AUTH_PASSWORD: "1234" OTLP_AUTH_USERNAME: root@example.com OTLP_COLLECTOR_URL: http://127.0.0.1:5080/api/default/v1 - STORAGE_PATH: /app/storage + STORAGE_URL: file:///app/storage volumes: - ws-server-storage:/app/storage depends_on: From 59f4ab6368af5c6824dafe2e11c9c598f16f5334 Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Wed, 5 Aug 2026 14:06:53 +0800 Subject: [PATCH 2/4] fix nextest --- .mise/config.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.mise/config.toml b/.mise/config.toml index cb1a9eb6..c380da2e 100644 --- a/.mise/config.toml +++ b/.mise/config.toml @@ -61,7 +61,7 @@ cargo-binstall = "latest" cmake = "latest" "conda:openssl" = "3" conftest = "latest" -"github:nextest-rs/nextest" = "latest" +"github:nextest-rs/nextest" = { version = "latest", version_prefix = "cargo-nextest-" } taplo = "latest" watchexec = "latest" # uutils coreutils: the Rust multicall binary, invoked as `coreutils ` in tasks. From 14ef3051286323427e6eaa4013f1faeaff180b68 Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Wed, 5 Aug 2026 16:14:42 +0800 Subject: [PATCH 3/4] skip on windows --- services/storage/tests/s3_backend.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/services/storage/tests/s3_backend.rs b/services/storage/tests/s3_backend.rs index b79b7873..e8a9da3c 100644 --- a/services/storage/tests/s3_backend.rs +++ b/services/storage/tests/s3_backend.rs @@ -105,7 +105,27 @@ fn aws_env(port: u16) -> Vec<(&'static str, Option)> { ] } +// Skipped on Windows: the pinned rustfs 1.0.0-beta.12 cannot initialise its storage on Windows, so it never +// reaches quorum and answers every request with `503 Service not ready: waiting for storage_quorum` -- which +// object_store retries a handful of times and then surfaces, turning the PUT below into a 500. The rustfs cause +// is a self-inflicted sharing violation: on Windows it opens each guarded ancestor directory (`.rustfs.sys` and +// friends) with FILE_SHARE_READ only, then renames a freshly-written child into that guarded parent, and Windows +// rejects the parent write with ERROR_SHARING_VIOLATION (Win32 code 32) -- logged by rustfs as +// reliable_rename failed. src_file_path: "...\\.rustfs.sys\\", dst_file_path: "...\\.rustfs.sys\\format.json", +// err: Os { code: 32, ... "The process cannot access the file because it is being used by another process." } +// This is not init-only: the same guarded rename backs the object-commit path (rustfs `rename_all` / `rename_data`), +// so no store-an-object round-trip can pass on Windows with this build -- there is no pre-seed or wait that helps. +// Fixed upstream in rustfs PR https://github.com/rustfs/rustfs/pull/5663 (guarded dirs now share FILE_SHARE_WRITE +// too; related issue https://github.com/rustfs/rustfs/issues/5419), merged 2026-08-03 -- AFTER the beta.12 tag +// (2026-07-30), so no released rustfs contains it yet. Observed on our commit +// 59f4ab6368af5c6824dafe2e11c9c598f16f5334 at +// https://github.com/edge-toolkit/core/actions/runs/30980282749/job/92223040435 (default (windows-latest, 120)). +// Re-enable by dropping this attribute once the `rustfs` mise tool is bumped to a release that includes #5663. #[actix_rt::test] +#[cfg_attr( + windows, + ignore = "rustfs beta.12 cannot init storage on Windows (ERROR_SHARING_VIOLATION) -- see above" +)] async fn round_trips_put_and_get_through_the_s3_backend() { let volume = tempfile::tempdir().unwrap(); // Pre-create the bucket: rustfs adopts an existing volume directory but will not create one on demand. From 9694fe1d0f38188f7a2edff67b73cde3d245de46 Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Thu, 6 Aug 2026 04:37:14 +0800 Subject: [PATCH 4/4] fix llvm-cov segfault --- .mise/config.coverage.toml | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/.mise/config.coverage.toml b/.mise/config.coverage.toml index 43e2d9b3..07706664 100644 --- a/.mise/config.coverage.toml +++ b/.mise/config.coverage.toml @@ -248,9 +248,29 @@ cargo clean -p et-ws-wasm-agent --target wasm32-unknown-unknown driver="${CHROMEDRIVER:-$(mise which chromedriver)}" # Build + run cov-server itself instrumented so the launcher is not reported as untested. -# It uses native -Cinstrument-coverage (with nightly branch coverage, matching the rest of the pipeline) and -# flushes its counters to this .profraw on the clean exit that removing the marker below triggers. -RUSTFLAGS="-Cinstrument-coverage -Zcoverage-options=branch" cargo build -q -p et-ws-test-server --bin cov-server +# It uses native -Cinstrument-coverage and flushes its counters to this .profraw on the clean exit that removing +# the marker below triggers. +# +# Deliberately WITHOUT `-Zcoverage-options=branch`, unlike the rest of the pipeline: branch regions over async +# code make llvm-cov segfault while reading this binary's coverage map, so the `-object target/debug/cov-server` +# export below aborts the whole task with +# bash: line 98: 49175 Segmentation fault: 11 "$llbin/llvm-cov" export --format=lcov +# --instr-profile "$csp.profdata" -object target/debug/cov-server > "$csp.lcov" +# and no LLVM stack trace. Under lldb the frame is +# `llvm::coverage::CoverageMapping::getInstantiationGroups(llvm::StringRef) const + 412`, an EXC_BAD_ACCESS on a +# near-null address -- the open upstream bug https://github.com/llvm/llvm-project/issues/189169 ("Crash in +# llvm-cov for rust with async code and --branch option", minimal repro at +# https://github.com/mscharley/llvm-cov-crash). It reproduces identically under every llvm-cov on hand (the +# nightly, stable and previous-stable toolchains' copies), is unaffected by `-num-threads=1` or an 8x larger +# stack, and follows the object rather than the profile. +# cov-server crossed the threshold when it gained the storage service's object_store dependency tree (285 crates, +# a 10.4 MB __llvm_covfun), which is why the coverage lane passed on commit d8d42f9c and failed from +# 6e80f4decd7d7d78ec6bd770d52105f4bd5db15a onward -- e.g. +# https://github.com/edge-toolkit/core/actions/runs/30988267419/job/92248023234. +# Dropping branch regions for this one binary keeps its line coverage and costs only its own BRDA records; every +# other crate still gets branch coverage via `a_wasm_cov_instr` / the show-env wrapper. Restore the flag here +# once #189169 is fixed and the toolchain ships it. +RUSTFLAGS="-Cinstrument-coverage" cargo build -q -p et-ws-test-server --bin cov-server LLVM_PROFILE_FILE="{{ config_root }}/$covdir/cov-server-%p.profraw" ./target/debug/cov-server "$covdir/server-ready" & server_pid=$! trap 'kill "$server_pid" 2>/dev/null || true' EXIT