diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6a4ab0e5..e47d4c19 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -48,6 +48,70 @@ jobs: - '**' - '!**.md' + # Everything else in CI runs on `stable` (see rust-toolchain.toml), so the MSRV + # promised to downstream users needs its own gate. This job is the only place + # the MSRV toolchain is used; it reads the version from the workspace + # Cargo.toml so `rust-version` stays the single source of truth. + # + # Scope: build only, no `--all-targets`. The promise is that consumers can + # *build* the published crates with the MSRV, not that our test suite runs + # there — dev-dependencies are free to require a newer compiler. + # + # Linux-only: MSRV regressions almost always come from a dependency raising its + # own `rust-version`, which is platform-independent. Running the full OS matrix + # would triple the cost to catch only platform-gated regressions (e.g. a + # windows-sys bump), which the stable `test` matrix would surface anyway once + # the dependency reaches a release we build. + msrv: + name: MSRV build + needs: changes + if: needs.changes.outputs.src == 'true' + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Read MSRV from Cargo.toml + id: msrv + run: | + set -euo pipefail + version=$(awk -F'"' '/^rust-version *=/ { print $2; exit }' Cargo.toml) + if [ -z "$version" ]; then + echo "::error file=Cargo.toml::could not parse rust-version" + exit 1 + fi + echo "Detected MSRV: $version" + echo "version=$version" >> "$GITHUB_OUTPUT" + + - name: Remove the runner's bundled Rust toolchain + run: rustup toolchain remove stable 2>/dev/null || true + + - uses: actions-rust-lang/setup-rust-toolchain@166cdcfd11aee3cb47222f9ddb555ce30ddb9659 # v1.17.0 + with: + toolchain: ${{ steps.msrv.outputs.version }} + target: wasm32-unknown-unknown + cache-shared-key: ${{ runner.os }}-msrv + cache-bin: false + rustflags: "" + + # ref-tests and ic-utils-bindgen-tests are the only `publish = false` + # members; they depend on pocket-ic from the IC monorepo, whose MSRV runs + # far ahead of ours. Excluding (rather than listing the published crates) + # keeps a newly added published crate covered by default. + - name: Build with MSRV + run: | + cargo build --locked --workspace \ + --exclude ref-tests --exclude ic-utils-bindgen-tests + cargo build --locked --workspace \ + --exclude ref-tests --exclude ic-utils-bindgen-tests --all-features + + # Browser consumers build ic-agent for wasm at the MSRV too; mirrors the + # WASM step in lint.yml. + - name: Build with MSRV (WASM) + run: | + CARGO_TARGET_DIR=target/wasm cargo build --locked --target wasm32-unknown-unknown \ + -p ic-agent --features wasm-bindgen -p ic-utils + # Workspace tests for every crate except ref-tests, on all three OSes. Because # each crate is tested from its own directory, the heavy pocket-ic dependency # (only used by ref-tests) never compiles here, keeping this job's cache small. @@ -199,8 +263,15 @@ jobs: # CARGO_TARGET_DIR=target/wasm keeps wasm artifacts under ./target, so # rust-cache (which caches ./target) still picks them up. + # + # --lib restricts this to the lib target's #[wasm_bindgen_test] tests. Newer + # toolchains also run doctests for wasm targets (1.88 skipped them), and + # ic-agent's doctests cannot compile there: they use #[tokio::main], and + # tokio is deliberately a dev-dependency only under + # cfg(not(target_family = "wasm")). Doctests are covered on the host by the + # `test` job above; running them inside a headless browser adds nothing. - name: Run Tests (WASM) - run: CARGO_TARGET_DIR=target/wasm wasm-pack test --chrome --headless ic-agent --features wasm-bindgen + run: CARGO_TARGET_DIR=target/wasm wasm-pack test --chrome --headless ic-agent --features wasm-bindgen --lib aggregate: name: test:required @@ -208,8 +279,11 @@ jobs: # skipped required check counts as passing for branch protection. if: always() && needs.changes.outputs.src == 'true' runs-on: ubuntu-latest - needs: [changes, test, ref_tests, wasm] + needs: [changes, msrv, test, ref_tests, wasm] steps: + - name: Check MSRV result + if: ${{ needs.msrv.result != 'success' }} + run: exit 1 - name: Check test result if: ${{ needs.test.result != 'success' }} run: exit 1 diff --git a/Cargo.toml b/Cargo.toml index 315fafa4..79669016 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,9 +17,16 @@ version = "0.49.2" authors = ["DFINITY Stiftung "] edition = "2021" repository = "https://github.com/dfinity/agent-rs" -# MSRV -# Avoid updating this field unless we use new Rust features -# Sync rust-version in rust-toolchain.toml +# MSRV — the single source of truth. The `msrv` job in +# .github/workflows/test.yml parses this line to pick its toolchain, so bumping +# the MSRV is a one-line change here. +# +# Avoid updating this field unless we need new Rust features, or a dependency +# forces it. It is a compatibility promise to downstream users: raise it only in +# a minor (not patch) release, and note it in CHANGELOG.md. +# +# rust-toolchain.toml is intentionally NOT pinned to this version; it tracks +# stable. rust-version = "1.88.0" license = "Apache-2.0" diff --git a/README.md b/README.md index 2133555d..094afd74 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,31 @@ We use `cargo` to build this repo. Make sure you have rust stable installed. To cargo build ``` +### Rust toolchain and MSRV +Two different Rust versions are in play, and they are deliberately decoupled: + +| | Where | Value | +|---|---|---| +| Development toolchain | `rust-toolchain.toml` | `stable` | +| MSRV | `rust-version` in the workspace `Cargo.toml` | pinned | + +Development, `cargo fmt`, `cargo clippy` and all CI jobs except one use the +latest **stable** release. The **MSRV** is the oldest compiler the published +crates are guaranteed to build with, and is enforced by the `msrv` job in +[`.github/workflows/test.yml`](.github/workflows/test.yml), which reads the +version out of `Cargo.toml`. That job builds the published crates only — the +MSRV covers building the libraries, not running our test suite, so +dev-dependencies may require a newer compiler. + +Consequences worth knowing: + +* Bumping the MSRV is a one-line change to `rust-version`. Do it only when we + need newer Rust features or a dependency forces it, in a minor (not patch) + release, with a CHANGELOG entry. +* Because clippy tracks stable, a new Rust release can introduce lints that fail + CI on an otherwise untouched branch. The fix is a small lint-cleanup PR, not an + MSRV or toolchain change. + ## Testing There are two suites of tests that can be executed from this repo; the regular cargo tests and the ic-ref tests. In order to run the ic-ref tests, you will need a running local reference diff --git a/ic-agent/http_mock_service_worker.js b/ic-agent/http_mock_service_worker.js index dacf7404..5e34cb12 100644 --- a/ic-agent/http_mock_service_worker.js +++ b/ic-agent/http_mock_service_worker.js @@ -38,10 +38,31 @@ async function getMock(nonce) { }); } +// Handle one request at a time. +// +// Both the `hits` counter and the route list are read-modify-write cycles over a +// single IndexedDB record: `getMock` reads the whole record in one transaction, +// the handler mutates its copy, and `setMock` writes the whole record back in +// another. Two requests in flight against the same mock therefore both read the +// pre-state and the second write silently discards the first one's mutation. +// +// That is not hypothetical: `Agent::query` issues its `query` and its +// `read_state` concurrently via `try_join!`, so a certifying-agent test reliably +// has two overlapping requests and can lose one of the two hit increments. There +// is nothing to gain from serving these concurrently — the responses come from +// canned data — so serialize the handlers and keep each cycle atomic. +let tail = Promise.resolve(); +function serialized(fn) { + const run = tail.then(fn); + // Keep the chain alive regardless of how this handler settles. + tail = run.then(() => {}, () => {}); + return run; +} + // Status codes are chosen to avoid being picked up as successes by tests expecting a 404 or 500. self.addEventListener("fetch", (event) => { - event.respondWith((async () => { + event.respondWith(serialized(async () => { try { const request = event.request; const url = new URL(request.url); @@ -83,7 +104,7 @@ self.addEventListener("fetch", (event) => { } catch (e) { return new Response(e.toString(), { status: 503 }); } - })()) + })); }); self.addEventListener("activate", (event) => { diff --git a/ic-agent/src/agent/agent_test.rs b/ic-agent/src/agent/agent_test.rs index f3f4c4c9..25fc8995 100644 --- a/ic-agent/src/agent/agent_test.rs +++ b/ic-agent/src/agent/agent_test.rs @@ -878,7 +878,10 @@ mod mock { pub async fn assert_mock(nonce: String) { let hits = get_hits(&nonce).await; - assert!(hits.values().all(|x| *x > 0)); + assert!( + hits.values().all(|x| *x > 0), + "some mocked routes were never hit: {hits:?}" + ); } pub async fn assert_single_mock(method: &str, path: &str, nonce: &String) { diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 823bf7af..dfcd8a60 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,7 +1,14 @@ [toolchain] -# MSRV -# Avoid updating this field unless we use new Rust features -# Sync rust-version in workspace Cargo.toml -channel = "1.88.0" +# Development toolchain — NOT the MSRV. +# +# We deliberately track the latest stable release here so that local builds, +# rust-analyzer, clippy and rustfmt all use a current compiler. Pinning this to +# the MSRV instead makes the repo progressively harder to work in as the MSRV +# ages (rust-analyzer refuses to work with sufficiently old toolchains) and +# hides new clippy/rustc diagnostics until the MSRV is bumped. +# +# The MSRV is `rust-version` in the workspace Cargo.toml, and is enforced by the +# `msrv` job in .github/workflows/test.yml, which reads it from there. +channel = "stable" components = ["rustfmt", "clippy"] targets = ["wasm32-unknown-unknown"]