fix(multipart): serialize the part commit per uploadId to prevent mixed-generation shards (backlog#853)
Concurrently re-transmitting the same part could land two shard
generations across different disks: each shard is individually
bitrot-valid, so the corruption surfaces only as a silent mix at read
time.
The hazard is confined to rename_part, where two temp parts are moved
cross-disk onto the SAME final part path — interleaving there can leave
shards from two generations. Each concurrent stream already writes to its
own unique temp dir, so the encode/stream phase never conflicts and must
stay lock-free: holding a lock across it would serialize slow re-transmits
of the same part, break the S3 'last finisher wins' semantics, and cause
UploadPart lock-acquire timeouts (ServiceUnavailable).
Scope a write lock to the uploadId namespace around only the rename_part
commit, so each commit is atomic across disks and the last committer wins
consistently. Mirrors MinIO's per-uploadID NS lock; disjoint from the
object lock held by complete_multipart_upload (no lock-ordering cycle).
Honors opts.no_lock.
The pre-delete-before-rename half of backlog#853 was already fixed in
#4316; this completes the issue.
Adds an end-to-end regression test: six concurrent resends of the same
part must all succeed (no lock-acquire timeout), exactly one generation
stays visible, and the reassembled object equals one intact generation.
Closesrustfs/backlog#853
fix(heal): remove useless .into() conversion in heal error path
Clippy flagged a useless_conversion lint at heal.rs:491 where
is redundant because is already of type .
Second TODO-convergence round over the current tree (backlog#646). All
line numbers in the old inventory had gone stale after the set_disk /
diagnostics / cluster refactors, so this re-scans and reduces the marker
count from 144 to 99.
STALE removals (comment describes already-implemented behavior, or dead
commented-out blocks) across ecstore (set_disk ops/core, store,
cluster/rpc, bucket/metadata_sys, services), iam, filemeta, s3select and
rustfs auth/object_usecase. No behavior change.
Safe fills, each verified:
- filemeta: replication_info_equals now also compares
replication_state_internal (function currently has no callers; adds a
regression test).
- bitrot: drop the confirmed-unused `_want` parameter from bitrot_verify
and the now-unused `sum` on LocalDisk::bitrot_verify, removing a
Bytes::copy_from_slice allocation. Streaming verify uses the file's
embedded per-shard hash, never the passed sum.
- signer: rename v4_ignored_headers -> V4_IGNORED_HEADERS and drop the
non_upper_case_globals allow.
- admin/heal: test_decode was #[ignore]d and used serde_urlencoded on a
JSON body (would panic); rewire to serde_json::from_slice to match the
production decode path, add assertions, un-ignore.
Verified: cargo fmt; cargo check on touched crates; tests pass
(filemeta, signer, bitrot, heal::test_decode); arch guardrail scripts
pass.
fix(ecstore): reject renewing a misplaced drive into the wrong set (backlog#799 B19)
`renew_disk` located the drive's (set, disk) position from its own format via
`find_disk_index`, but never checked that the resolved `set_idx` matched this
`SetDisks`' own `set_index` before inserting the drive into `self.disks`. A
drive whose format places it in another set would be claimed by this set too,
so two sets could manage the same drive and degrade together.
Skip (with a warning) when `set_idx != self.set_index`.
Refs backlog#799 (B19), tracked in rustfs/backlog#863.
heal_object wrote healed shards to .rustfs/tmp/<uuid>/ and only removed that tmp
dir on the success path (the final delete_all). Three early exits after the tmp
shards were written leaked the dir:
- `erasure.heal(...)?` failing midway,
- the `disks_to_heal_count == 0` early return, and
- the `?` on the post-rename remote data-dir delete.
Add the tmp cleanup before the first two, and downgrade the remote data-dir
cleanup failure to a warning (the healed shard is already renamed into place, so
that cleanup failing must not abort the heal or leak the tmp shards).
Refs backlog#799 (B20), tracked in rustfs/backlog#863.
`MultiWriter` recorded per-writer failures only in a private `errs` vector and
nulled the failed writer locally, but `put_object` never used that to prune the
disk set: `rename_data` ran over the full `shuffle_disks`, so a disk that took a
short/failed write still had its truncated shard renamed into place and counted
as an online disk. The object then claimed N good shards while one was
short/corrupt, so a single later disk failure could drop it below reconstructable
quorum — silent data loss.
- `write_shard`: null the writer on a generic write error too (not only on
`ShortWrite`), so a failed writer is uniformly represented as `None` — matching
`shutdown_writer`, which already does this.
- `put_object`: after encode, drop every disk whose writer failed
(`drop_failed_writer_disks`) from `shuffle_disks` before `rename_data`, and
re-check write quorum over the survivors. `rename_data` already re-checks
quorum and rolls back if too few disks remain, so excluded disks are neither
committed nor counted (MinIO sets failed writers to nil before `renameData`).
Adds unit tests for the exclusion/quorum accounting.
Refs backlog#799 (B3).
P6 of the SetDisks God-Object split (tracking backlog#815, issue backlog#821;
depends on P5 #4288). Relocate the core object read/write hot-path contract
impls out of the set_disk/mod.rs God-Object into their own module home:
- impl ObjectIO for SetDisks (~1,010 lines) -> set_disk/ops/object.rs
- impl ObjectOperations for SetDisks (~1,137 lines) -> set_disk/ops/object.rs
- Registered pub(crate) mod object; under set_disk/ops.
Pure move — zero logic change. Both contracts stay implemented for SetDisks, so
their EcstoreObjectIO / EcstoreObjectOperations associated-type bounds are
unchanged and the contract-compat tests still guard them. Method bodies are
moved verbatim: a whitespace-insensitive token-stream diff of the two impl
blocks (with their #[async_trait::async_trait] attributes) against the pre-move
mod.rs source is byte-identical (5,754 tokens each) — NO visibility widening was
required, because the impls reach SetDisks helpers and the P5 io_primitives
through inherent self./Self:: calls that resolve across modules unchanged. The
two inherent impl SetDisks blocks that sat between the trait impls (lock batch
helpers) remain in mod.rs, verified present exactly once and uncorrupted.
The issue's borrow/Arc-clone-avoidance optimization is intentionally deferred:
it is a perf-sensitive change requiring the #738 benchmark and would risk the
'byte-level behavior unchanged' acceptance; this PR delivers the relocation.
Verification:
- cargo check / clippy -D warnings -p rustfs-ecstore --all-targets: clean
- cargo test -p rustfs-ecstore --lib: 1841 passed, 0 failed
- all five arch guard scripts: pass
- token-stream diff of moved impls vs original: identical (mod.rs diff is a pure
relocation; ObjectIO/ObjectOperations each defined exactly once post-move)
P1 of the SetDisks split (tracking backlog#815, depends on P0 backlog#816).
Give the Heal operation family its own module home: relocate set_disk/heal.rs
to set_disk/ops/heal.rs and move the HealOperations storage-api contract impl
beside its inherent helpers. The contract stays implemented for SetDisks so its
associated-type bounds are unchanged (ecstore_contract_compat_test still
covers it).
Method bodies are moved unchanged. The four inherent helpers widen from
pub(super) to pub(in crate::set_disk) to preserve their exact prior visibility
from the deeper module. get_pool_and_set now reads topology through
SetDisksCtx to keep the Heal family aligned with the P0 borrow pattern; the
read is provably identical (ctx.format()/pool_index() alias the core fields).
Runtime behavior is unchanged.
Zhengchao An