ci: track stable in rust-toolchain.toml, enforce MSRV in its own job (#741)

* 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) <noreply@anthropic.com>

* fix(ic-agent): make the wasm test mock hit-counter atomic, skip wasm doctests

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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: lwshang

.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,17 +263,27 @@ 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
     # Runs only when the test jobs ran; on docs-only changes this skips, and a
     # 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

Cargo.toml

@@ -17,9 +17,16 @@ version = "0.49.2"
 authors = ["DFINITY Stiftung <sdk@dfinity.org>"]
 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"
 

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

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) => {

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) {

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"]