From 6d6cbf75790f8dc7f52458452d385804a68a92ee Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Tue, 4 Aug 2026 09:18:19 -0400 Subject: [PATCH 1/2] ci: track stable in rust-toolchain.toml, enforce MSRV in its own job rust-toolchain.toml was pinned to the MSRV (1.88.0), so every local build, rust-analyzer, clippy and every CI job used a compiler nine releases behind stable. That makes the repo progressively harder to work in as the MSRV ages (rust-analyzer now refuses sufficiently old toolchains) and hides new rustc/clippy diagnostics until the MSRV is bumped. Split the two concerns, matching the setup in dfinity/cdk-rs: - rust-toolchain.toml tracks `stable`. - `rust-version` in the workspace Cargo.toml stays at 1.88.0 and becomes the single source of truth for the MSRV. - A new `msrv` job in test.yml parses `rust-version` out of Cargo.toml and builds with that toolchain, so bumping the MSRV is a one-line change and cannot drift out of sync with CI. The msrv job covers the published crates only (ref-tests and ic-utils-bindgen-tests depend on pocket-ic from the IC monorepo, whose MSRV runs far ahead of ours) and builds without --all-targets: the promise is that consumers can build the libraries at the MSRV, not that our test suite runs there. It gates on the existing `changes` filter and reports through the existing `test:required` aggregate, so no branch-protection changes are needed. Verified before landing: cargo fmt, `cargo hack clippy --each-feature`, `clippy --all-targets --all-features -D warnings`, the wasm clippy target and `RUSTDOCFLAGS=-Dwarnings cargo doc` all pass on stable 1.97, and the new msrv commands pass on 1.88.0. No source changes were needed. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 69 +++++++++++++++++++++++++++++++++++++- Cargo.toml | 13 +++++-- README.md | 25 ++++++++++++++ rust-toolchain.toml | 15 ++++++--- 4 files changed, 114 insertions(+), 8 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6a4ab0e5..37b56076 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. @@ -208,8 +272,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/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"] From d295c59bbc20bcce843f6f5d7dd127b12eef3c10 Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Tue, 4 Aug 2026 09:38:34 -0400 Subject: [PATCH 2/2] fix(ic-agent): make the wasm test mock hit-counter atomic, skip wasm doctests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving rust-toolchain.toml to `stable` surfaced two latent problems in the WASM job. Both are test-harness issues; no library code changes. 1. `agent::agent_test::no_cert` failed with "some mocked routes were never hit". The service-worker mock counts hits with a read-modify-write across two IndexedDB transactions: getMock reads the whole record, the handler mutates its copy, setMock writes it back. `Agent::query` issues its `query` and its `read_state` concurrently via `try_join!`, so both handlers read the pre-state and the second write discards the first one's increment. `no_cert` is the only test that combines concurrent requests with assert_mock (which requires *every* route to be hit), so it is the only one that noticed. Confirmed rather than inferred: the counter for the `query` route reads 0 even though the test's MissingSignature assertion — which requires the query response to have been served — passes. The request happened; only its increment was lost. Fixed by serializing the service worker's handlers; the responses are canned data, so there is nothing to gain from overlapping them. Locally: fails 2/2 on stable 1.97 and passes 2/2 on 1.88 before the fix, passes 3/3 on both after. assert_mock now prints the hit map on failure. The original assertion gave no indication of which route was missed. 2. Newer toolchains run doctests for wasm targets, which 1.88 skipped. ic-agent's doctests use `#[tokio::main]` and tokio is deliberately a dev-dependency only under cfg(not(target_family = "wasm")), so they cannot compile there: "cannot find module or crate `tokio`". Pass --lib so the browser run covers the #[wasm_bindgen_test] tests it is meant to cover; the host `test` job already runs the doctests. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 9 ++++++++- ic-agent/http_mock_service_worker.js | 25 +++++++++++++++++++++++-- ic-agent/src/agent/agent_test.rs | 5 ++++- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 37b56076..e47d4c19 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -263,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 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) {