Repository navigation
chore: rustc: 1.97.1 -> 1.98.1 - #11534
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is a consistent, isolated toolchain version bump plus a mechanically updated benchmark baseline with no remaining references to the old version.
Pull request overview
Upgrades the repository’s Rust toolchain to 1.98.1 across both the Rust toolchain file and Bazel rules_rust toolchain configuration, and updates the NNS governance canbench baseline to reflect instruction-count shifts from the new compiler/codegen.
Changes:
- Bump
rust-toolchain.tomlchannel from1.97.1to1.98.1. - Bump Bazel
rules_rusttoolchainversionsfrom1.97.1to1.98.1. - Regenerate
rs/nns/governancecanbench baseline numbers (YAML) to match new measurements.
File summaries
| File | Description |
|---|---|
rust-toolchain.toml |
Updates the Rust toolchain channel to 1.98.1. |
bazel/rust.MODULE.bazel |
Updates the Bazel Rust toolchain version to 1.98.1 to keep Bazel in sync. |
rs/nns/governance/canbench/canbench_results.yml |
Updates governance canbench instruction counts to the new baseline after the toolchain bump. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
basvandijk
marked this pull request as ready for review
September 10, 2026 13:04
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to 0c415e7. Security Overview
Detected Code Changes
|
basvandijk
dismissed
github-actions[bot]’s stale review
September 10, 2026 13:05
No canister behaviour changes
nmattia
approved these changes
Sep 10, 2026
schneiderstefan
approved these changes
Sep 10, 2026
basvandijk
force-pushed
the
bas/rust-1.98.1
branch
3 times, most recently
from
September 10, 2026 13:35
cb9659f to
fc825f0
Compare
|
✅ No security or compliance issues detected. Reviewed everything up to 0c415e7. Security Overview
Detected Code Changes
|
bjoernek
approved these changes
Sep 12, 2026
Upgrade the rust toolchain from 1.97.1 to 1.98.1. See: https://blog.rust-lang.org/2026/08/20/Rust-1.98.0/. The clippy fixes this needs are in the PR below this one in the stack, so this PR is just the version bump in `rust-toolchain.toml` and `bazel/rust.MODULE.bazel`, plus the NNS governance canbench baseline that the new codegen shifts. Only one benchmark moved by more than the 5% noise threshold of //rs/nns/governance:governance-canbench_test: | status | name | calls | ins | ins Δ% | |--------|----------------------------|-------|-------|---------| | + | neuron_metrics_calculation | | 3.67M | +7.52% | so `canbench/canbench_results.yml` was regenerated with `bazel run //rs/nns/governance:governance-canbench_update`. The two icrc1 ledger canbench tests stay within their noise threshold, so their results files are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerated with `bazel run //rs/bitcoin/ckbtc/minter:ckbtc_minter_canbench_update` after CI reported //rs/bitcoin/ckbtc/minter:ckbtc_minter_canbench_test as failing on the new toolchain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI reported //rs/recovery/subnet_splitting:integration_test as failing on the new toolchain. There is no `_update` target for it - the expected values are hardcoded in the test - so they were re-pinned by hand from a single 1.98.1 reference run. What actually changed is the *partition*, not the workload: every total is identical (canisters 20, ingress messages 39, remote 10, local 28, http outcalls 10, heartbeats 694) and the total instructions moved by 0.0016%. `split-finder` solves a symmetric MILP over load samples derived from measured execution, and that tiny shift was enough to flip a near-tied optimum to a different partition - both of which balance instructions ~50/50, so both are equally optimal for the solver's objective. Checked before changing anything: - The test is deterministic, not flaky: 5/5 uncached runs on 1.98.1 gave byte-identical values. - On 1.97.1 this machine reproduces the currently committed values exactly (14/6, 26/13, 6/4, 6335280/2927104) and the test passes, so the environment agrees with CI and the new values should too. - All ten values come from one reference run and are written in a single consistent source/destination orientation, as `assert_eq_oriented!` requires. Note for the owners: because this pins the exact solution of a degenerate optimisation, it will keep flipping on unrelated changes. Making it robust (e.g. asserting on totals and on the ~50/50 instruction balance, which both splits satisfy) would be worth doing separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
basvandijk
force-pushed
the
bas/rust-1.98.1
branch
from
September 12, 2026 18:34
79ce2fe to
0c415e7
Compare
blind-oracle
approved these changes
Sep 14, 2026
mbjorkqvist
approved these changes
Sep 14, 2026
basvandijk
added a commit
that referenced
this pull request
Sep 14, 2026
## Problem
The `Bazel Build with RBE` step of the `CI RBE Evaluation` workflow
times out on
essentially every push to master:
```
[36,930 / 36,936] [Sched] Compiling Rust bin icp_features (2 files); 5239s ... (3 actions, 0 running)
Error: The action 'Bazel Build with RBE' has timed out after 90 minutes.
```
The action is not compiling slowly — it never starts. `[Sched]` is
Bazel's marker
for *queued on the remote executor*, the number next to it is queue
time, and
`0 running` confirms nothing is executing. The build reaches 36,926 of
36,936
actions in 46 minutes and then spends the remaining 44 waiting. When one
of these
actions did eventually get a slot it finished in about a second:
```
11:58:23 [36,926 / 36,936] [Sched] Compiling Rust bin unix (2 files); 4439s ... (5 actions, 0 running)
11:58:46 [36,926 / 36,936] Compiling Rust bin tests (2 files); 0s remote, remote-cache ... (5 actions, 1 running)
11:58:57 [36,928 / 36,936] Compiling Rust bin unix (2 files); 1s remote, remote-cache ...
```
## Cause
Two things combine:
1. **A target-level `exec_properties` applies to every action the target
owns**,
not just the test run. So on a `rust_test`, `exec_properties = {"cpu":
"16"}`
also lands on the `Rustc` action that compiles the test binary. The
`cpu:n`
tag that #10786 replaced only ever affected the test action, so that PR
widened the reservation without meaning to.
2. **Namespace RBE workers run 16 actions concurrently** (hence
`--jobs=240` for
15 workers). An action asking for `cpu: 16` therefore only fits on a
completely drained worker, which under cluster load essentially never
happens.
The `cpu: 8` targets in the same build are unaffected and compiled in
16–27s
throughout, which confirms the threshold is the worker size rather than
target
complexity:
| target | cpu | outcome |
| --- | --- | --- |
| `pocket-ic-server-head-nns` | 8 | compiled in 25s |
| `neuron_test` | 8 | compiled in 16s |
| `upgrade_canister_test` | 8 | compiled in 0–11s |
| `icp_features`, `icp_features-head-nns` | 16 | starved 87 min →
timeout |
This has been failing on every master push since `rustc: 1.97.1 ->
1.98.1`
(#11534) landed, which invalidated the Rust compile cache and forced
these test
binaries to be rebuilt. Because the compiles never finish they never
populate the
CAS, so each run retries from scratch. Earlier failures hit the `Bazel
Test with
RBE` step with the same signature (`Testing //packages/pocket-ic:slow`
queued for
7154s).
The non-RBE `Bazel Test All` job is unaffected: it runs locally on one
large
runner, where `cpu: 16` is just a local core reservation.
## Fix
Prefix the property with `test.` so it is scoped to the `test` exec
group, i.e.
to the test action only, restoring the semantics of the original `cpu:n`
tag.
This is applied to every `cpu` execution property, not only the ones
that
currently starve — a compile action never needed the reservation, and
reserving
cores for it only wastes RBE capacity.
## Verification
- `bazel build //... --nobuild` analyzes all 3348 targets successfully,
which
also confirms `test` is a valid exec group for every rule touched here
(`rust_test`, `rust_ic_test`, the `rust_test_suite` variants and
`sh_test`);
an unknown exec group fails analysis with *"Tried to set exec_properties
for
non-existent exec groups"*.
- `bazel aquery //packages/pocket-ic:icp_features` shows the reservation
has
moved off the compile action:
```
Rustc cpu-related execution_info: []
TestRunner cpu-related execution_info: [{'key': 'cpu', 'value': '16'}]
```
- `bazel run //:buildifier` reports no formatting changes.
## Follow-up
This does not by itself fix the `Bazel Test with RBE` step, where the
test action
still asks for a whole worker and can starve the same way. Lowering
those
reservations below the worker size is left to a follow-up. It may also
be worth
raising the whole-worker starvation with Namespace — a scheduler with
reservation
or backfill shouldn't starve a 16-CPU request indefinitely.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrade the rust toolchain from 1.97.1 to 1.98.1. See:
https://blog.rust-lang.org/2026/08/20/Rust-1.98.0/.
The
cargo clippyfixes this needs are in the PR below this one in the stack, so this PR isjust the version bump in
rust-toolchain.tomlandbazel/rust.MODULE.bazel, plus the NNSgovernance canbench baseline that the new codegen shifts.
Only one benchmark moved by more than the 5% noise threshold of
//rs/nns/governance:governance-canbench_test:neuron_metrics_calculationso
rs/nns/governance/canbench/canbench_results.ymlwas regenerated withbazel run //rs/nns/governance:governance-canbench_update. The two icrc1 ledger canbench testsstay within their noise threshold, so their results files are unchanged.
CI then flagged two more targets, both fixed here:
//rs/bitcoin/ckbtc/minter:ckbtc_minter_canbench_test— regenerated withbazel run //rs/bitcoin/ckbtc/minter:ckbtc_minter_canbench_update(34 instruction counts).//rs/recovery/subnet_splitting:integration_test— this one has no_updatetarget; itsexpected values are hardcoded in the test, so all ten were re-pinned by hand from a single
1.98.1 reference run, in one consistent source/destination orientation.
What changed is the partition, not the workload. Every total is identical — canisters 20,
ingress messages 39, remote 10, local 28, http outcalls 10, heartbeats 694 — and total
instructions moved by 0.0016%.
split-findersolves a symmetric MILP over load samples derivedfrom measured execution, and that tiny shift was enough to flip a near-tied optimum to a
different partition; both balance instructions ~50/50, so both are equally optimal for the
solver's objective.
states_sizes_bytescanisters_installedingress_messages_executedremote_subnet_messages_executed_lower_boundlocal_subnet_messages_executed_upper_boundhttp_outcalls_executedheartbeats_and_global_timers_executedBefore changing the values I checked that this is a real shift and not a flake: the test is
deterministic (5/5 uncached runs on 1.98.1 give byte-identical values), and on 1.97.1 this
machine reproduces the currently committed values exactly and passes — so the environment
agrees with CI and the new values should too.
degenerate optimisation, it will keep flipping on unrelated changes. Asserting on the totals
and on the ~50/50 instruction balance — which both partitions satisfy — would make it robust.
No repin is needed: no third-party crate dependency changed, so the
Cargo.Bazel.*.lockfilesare unaffected.
🤖 Generated with Claude Code