*: Update rustc to update time crate#19367
Conversation
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes the nightly Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates TiKV’s Rust dependency set to address RUSTSEC-2026-0009 by upgrading the time crate and its transitive dependencies, which also necessitates bumping the pinned nightly toolchain and refactoring code away from the let_chains feature.
Changes:
- Bump Rust nightly toolchain and update
time(and related deps) inCargo.lock. - Remove
#![feature(let_chains)]across crates and refactorif let ... && ...usages into nested conditionals. - Add a workspace
[patch]override forraft-engineto a fork/branch to pick up a serde-related update.
Reviewed changes
Copilot reviewed 79 out of 80 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/txn/flow_controller/singleton_flow_controller.rs | Refactor let_chains usage to nested if. |
| src/storage/txn/actions/prewrite.rs | Refactor let_chains usage to nested if. |
| src/storage/mod.rs | Replace assert_matches! usage with assert!(matches!(...)) and drop import. |
| src/server/tablet_snap.rs | Refactor let_chains usage to nested if. |
| src/server/service/kv.rs | Refactor let_chains usage to nested if. |
| src/server/raftkv2/raft_extension.rs | Refactor let_chains in feed (introduces behavior change). |
| src/server/raftkv/raft_extension.rs | Refactor let_chains in feed (introduces behavior change). |
| src/server/lock_manager/waiter_manager.rs | Refactor let_chains usage to nested if. |
| src/server/engine_factory.rs | Refactor let_chains usage to nested if. |
| src/server/debug2.rs | Refactor let_chains usage to nested if. |
| src/server/debug.rs | Refactor let_chains usage to nested if. |
| src/lib.rs | Remove #![feature(let_chains)]. |
| src/coprocessor/endpoint.rs | Refactor let_chains usage to nested if. |
| src/config/mod.rs | Refactor let_chains usage to nested if / if-nesting. |
| rust-toolchain.toml | Bump pinned nightly toolchain date. |
| components/tikv_util/src/worker/pool.rs | Refactor while ... && let ... into loop+match. |
| components/tikv_util/src/lib.rs | Remove #![feature(let_chains)]. |
| components/test_raftstore/src/lib.rs | Remove #![feature(let_chains)]. |
| components/test_raftstore/src/cluster.rs | Refactor let_chains usage to nested if. |
| components/test_raftstore-v2/src/lib.rs | Remove #![feature(let_chains)]. |
| components/server/src/signal_handler.rs | Refactor let_chains usage to nested if. |
| components/server/src/lib.rs | Remove #![feature(let_chains)]. |
| components/resource_control/src/resource_limiter.rs | Refactor let_chains usage to nested if. |
| components/resource_control/src/lib.rs | Remove #![feature(let_chains)]. |
| components/resolved_ts/src/lib.rs | Remove #![feature(let_chains)]. |
| components/raftstore/src/store/worker/split_controller.rs | Refactor let_chains usage to nested if. |
| components/raftstore/src/store/worker/split_check.rs | Refactor let_chains usage to nested if (adds explicit init path). |
| components/raftstore/src/store/util.rs | Refactor let_chains usage to nested if. |
| components/raftstore/src/store/simple_write.rs | Replace debug_assert_matches! with debug_assert!(matches!(...)). |
| components/raftstore/src/store/fsm/peer.rs | Refactor let_chains usage to nested if. |
| components/raftstore/src/store/async_io/write.rs | Refactor let_chains usage to nested if. |
| components/raftstore/src/lib.rs | Remove #![feature(let_chains)]. |
| components/raftstore/src/coprocessor/region_info_accessor.rs | Refactor let_chains usage to nested if. |
| components/raftstore/src/coprocessor/config.rs | Replace let_chains control flow with match to keep logic. |
| components/raftstore-v2/src/worker/tablet.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/unsafe_recovery/report.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/unsafe_recovery/force_leader.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/unsafe_recovery/destroy.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/unsafe_recovery/demote.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/unsafe_recovery/create.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/ready/snapshot.rs | Replace assert_matches! with assert!(matches!(...)) and refactor let_chains. |
| components/raftstore-v2/src/operation/ready/mod.rs | Refactor multi-let chain into tuple match. |
| components/raftstore-v2/src/operation/ready/apply_trace.rs | Refactor let_chains usage to nested if with early return. |
| components/raftstore-v2/src/operation/query/capture.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/misc.rs | Refactor let_chains around send/shutdown handling. |
| components/raftstore-v2/src/operation/life.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/command/write/ingest.rs | Refactor let_chains around send/shutdown handling. |
| components/raftstore-v2/src/operation/command/mod.rs | Refactor let_chains usage to nested if (and move perf reporting into block). |
| components/raftstore-v2/src/operation/command/admin/mod.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/command/admin/merge/rollback.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/command/admin/merge/prepare.rs | Refactor let_chains; introduces unconditional merge_context_mut() call. |
| components/raftstore-v2/src/operation/command/admin/merge/mod.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/operation/command/admin/merge/commit.rs | Refactor let_chains usage to nested if with early return. |
| components/raftstore-v2/src/operation/command/admin/compact_log.rs | Refactor let_chains usage to nested if. |
| components/raftstore-v2/src/lib.rs | Remove #![feature(let_chains)]. |
| components/raftstore-v2/src/fsm/store.rs | Refactor let_chains (introduces logic regression in set_region). |
| components/raftstore-v2/src/batch/store.rs | Refactor let_chains usage to nested if. |
| components/pd_client/src/lib.rs | Remove #![feature(let_chains)]. |
| components/in_memory_engine/src/write_batch.rs | Refactor let_chains usage to nested if. |
| components/in_memory_engine/src/region_stats.rs | Refactor let_chains usage to nested if. |
| components/in_memory_engine/src/region_manager.rs | Replace assert_matches! with assert!(matches!(...)) and refactor let_chains. |
| components/in_memory_engine/src/lib.rs | Remove #![feature(let_chains)]. |
| components/in_memory_engine/src/background.rs | Refactor let_chains usage to nested if. |
| components/hybrid_engine/src/observer/load_eviction.rs | Refactor let_chains usage to nested if. |
| components/hybrid_engine/src/lib.rs | Remove #![feature(let_chains)]. |
| components/engine_traits/src/lib.rs | Remove #![feature(let_chains)]. |
| components/engine_test/src/lib.rs | Remove #![feature(let_chains)] and refactor let_chains. |
| components/engine_rocks/src/util.rs | Refactor let_chains usage to nested if / early returns. |
| components/engine_rocks/src/rocks_metrics.rs | Refactor let_chains usage to nested if. |
| components/engine_rocks/src/raft_engine.rs | Refactor let_chains usage to nested if. |
| components/engine_rocks/src/misc.rs | Refactor let_chains usage to nested if. |
| components/engine_rocks/src/lib.rs | Remove #![feature(let_chains)]. |
| components/encryption/src/manager/mod.rs | Refactor let_chains usage to nested if. |
| components/encryption/src/lib.rs | Remove #![feature(let_chains)]. |
| components/crossbeam-skiplist/src/base.rs | Adjust unsafe tower indexing to satisfy newer compiler constraints. |
| cmd/tikv-ctl/src/main.rs | Remove #![feature(let_chains)]. |
| cmd/tikv-ctl/src/executor.rs | Refactor let_chains usage to nested if. |
| clippy.toml | Move clippy config keys to top of file. |
| Cargo.toml | Add [patch] override for raft-engine to fork/branch. |
| Cargo.lock | Update locked versions (notably time to 0.3.47, chrono, serde, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
components/raftstore-v2/src/operation/command/admin/merge/prepare.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Around line 229-230: The Cargo.toml patch overrides raft-engine with a
personal fork (raft-engine = { git = "https://github.com/v01dstar/raft-engine",
branch = "update-serde" }), which must be temporary; update the patch to point
back to the official tikv/raft-engine repo once upstream PR tikv/raft-engine#390
is merged (or remove the patch entirely), and add a clear TODO comment next to
this patch referencing the upstream PR and the requirement to revert to official
sources to avoid supply-chain risk—mirror the style of the existing TODO around
the nearby lines (226-227) so reviewers can track the temporary override and the
condition for its removal.
In `@components/raftstore-v2/src/operation/command/admin/compact_log.rs`:
- Around line 534-546: The code currently sets last_compacted_idx even when
store_ctx.engine.gc(...) fails; change the flow in compact_log handling so that
after calling store_ctx.engine.gc(self.region_id(), 0, index,
self.state_changes_mut()) you only call
self.compact_log_context_mut().set_last_compacted_idx(index) when gc returns Ok
— i.e., move the set_last_compacted_idx call into the success branch (after the
call completes without Err) and leave it unchanged when Err(e) is returned
(keeping the error! logging in the Err arm using error!(... "err" => ?e)).
Ensure you still obtain index via self.compact_log_index() and preserve the
surrounding checks against storage().apply_trace().persisted_apply_index().
In `@rust-toolchain.toml`:
- Line 3: The rust-toolchain bump in rust-toolchain.toml (channel =
"nightly-2026-01-30") needs verification by running the standard lint/dev flow;
run `make clippy` to ensure the RUSTSEC fix and removal of let-chains produce no
warnings/errors, and if you want full checks run `make dev` to include
formatting + clippy; fix any resulting clippy or formatting failures (in the
crate modules, functions, or files changed by this toolchain bump) until both
commands succeed.
In `@src/server/raftkv/raft_extension.rs`:
- Around line 67-70: The current change only calls
self.router.send_raft_msg(msg) when key_message is true, which drops non-key
raft messages; change it so send_raft_msg is always called but keep the
conditional logging for key messages: always invoke
self.router.send_raft_msg(msg) and if it returns Err(e) then, if key_message is
true, call error! with region_id, msg_ty and err => ?e (preserve existing
contextual fields), otherwise swallow/ignore the error as before; update the
block around the key_message check and the send_raft_msg call (referencing the
key_message variable, self.router.send_raft_msg, and the existing error handling
identifiers region_id, msg_ty, and e).
🧹 Nitpick comments (3)
components/in_memory_engine/src/background.rs (1)
1296-1339: Indentation of the inner block appears off after the refactor.The body (lines 1298–1337) was originally at the correct indent level for a single combined
if. After splitting into two nestedifs, the body should be indented one additional level under the innerif let Some(e) = &self.rocks_engine. Please runmake formatto fix the indentation. As per coding guidelines, "Usemake formatto run code formatter for Rust source code".components/raftstore-v2/src/operation/unsafe_recovery/destroy.rs (1)
11-20: Indentation of the inner block needs adjustment.Lines 13–18 should be indented one more level inside the
if !state.is_abort()block. Please runmake format. As per coding guidelines, "Usemake formatto run code formatter for Rust source code".components/raftstore-v2/src/operation/unsafe_recovery/report.rs (1)
30-39: Same indentation issue as other refactored files.Lines 32–37 should be indented one more level under
if !state.is_abort(). Please runmake format. As per coding guidelines, "Usemake formatto run code formatter for Rust source code".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/raftstore-v2/src/fsm/store.rs`:
- Around line 66-99: The code currently only inserts into self.region_ranges
when prev is Some, so newly added initialized regions (prev == None) are never
recorded; move the insertion of (data_end_key(region.get_end_key()), version) ->
region_id out of the if let Some(prev) block so that if initialized is true you
always call self.region_ranges.insert(...) (use the existing initialized check
and the same assertion used now), keeping the existing prev-handling logic (the
asserts on prev_init and epoch/version checks) unchanged; update the block
around region_ranges.insert in the function where region, version, region_id,
initialized, data_end_key, and self.region_ranges are referenced to perform the
insert unconditionally when initialized is true.
|
Since |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/server/raftkv/raft_extension.rs`:
- Around line 67-73: Remove the stray extra closing brace that causes a syntax
error: locate the block where self.router.send_raft_msg(msg) is called and the
subsequent if key_message { … } logging branch (references: method
send_raft_msg, variable self.router, variable key_message, and msg_ty/region_id
logging) and delete the redundant `}` after that block so the braces for the if
let Err(e) and the if key_message match correctly.
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Rustc is already upgrade in this PR, and |
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
glorv
left a comment
There was a problem hiding this comment.
I saw in some modules, it replaces if-let-chains with nested if blocks, please investigate the root cause and revert these changes.
rest LGTM
| let time = | ||
| std::time::Instant::now() + limiter.consume_duration(1); | ||
| let _ = GLOBAL_TIMER_HANDLE.delay(time).compat().await; | ||
| if let Some(mut t) = registry.get(r) { |
There was a problem hiding this comment.
I saw many other changes are "replacing nested if with let-chains", but why here it replaced the if-let-chains back to nested if blocks?
There was a problem hiding this comment.
It depends on whether we "can" set a specific module's edition to 2024 in its Cargo.toml. After upgrading Rust nightly, to use let_chains, we have to set edition to 2024, feature(let_chains) no longer work. I set some of the modules to use 2024 edition, if the module requires minimum changes. For some modules like raftstore, setting edition to 2024 also force us to do massive styling changes related to borrow/lifetime, thats why I created a separate issue to track this in this PR's description
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gengliqi, glorv, zhangjinpeng87 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
There is the same clippy failure in release-8.5 branch. Shall we pick this PR to 8.5 or just ignore the "RUSTSEC-2026-0009" in release-8.5's |
close tikv#19355 Update time crate to fix RUSTSEC-2026-0009. Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
close tikv#19355 Update time crate to fix RUSTSEC-2026-0009. Signed-off-by: Yang Zhang <yang.zhang@pingcap.com> Signed-off-by: rishabh mittal <mittalrishabh@gmail.com>
What is changed and how it works?
Issue Number: Close #19355
What's Changed:
Updating time crate requires:
1.1. Requires removing let-chains usage unless edition is set to 2024,
feature(let_chains)does not work1.2. It is more "readable" removing all
let_chainsrather than bump Rust edition to 2024, I recommend we upgrade Rust edition to 2024 separately, and upgrade each module individually. created Update Rust edition to 2024 #19368 for tracking2.1 The raft-engine PR is Update serde, so TiKV can update time crate raft-engine#390
Can we separate this into multiple smaller PRs:
Yes and No. TiKV CI is failing, any PR requires fixing RUSTSEC, fixing RUSTSEC requires update nightly version, and here we are again...
Files need attentions:
Related changes
pingcap/docs/pingcap/docs-cn:Check List
Tests
Side effects
Release note
Summary by CodeRabbit
Refactor
Chores