The policy crate's Go path.Clean port and rustfs-utils' Windows-aware clean look like duplicates but are not interchangeable: S3 ARN/resource matching must treat backslashes as object-name data, never as separators, so adopting the utils version would change policy evaluation semantics on Windows — a security-adjacent behavior change. Record that judgment as bidirectional do-not-merge notes on both implementations, per the issue's adversarial ruling.
Comment-only change.
Ref rustfs/backlog#1833 (PR7).
BitrotErrorType (disk/error.rs) was constructed only by its own unit test: production bitrot mismatches never flow through it (they surface as DiskError::other strings). Delete the enum, its From<BitrotErrorType> for DiskError impl, the self-test, and the api facade re-export. The facade inventory doc does not name the type, so no doc change is needed.
DiskError::SourceStalled and DiskError::CrossDeviceLink are never constructed locally — they are reachable only through wire decoding and no current node sends them. Their decode arms stay per the cross-version compatibility constraint; each variant now carries a doc comment saying exactly that so the next dead-code sweep does not re-litigate them. Their consumer arms (heal classifier, batch processor) are left untouched — the values cannot appear, so removing the arms would be unobservable, and the heal classifier is pinned by the issue as do-not-touch.
Ref rustfs/backlog#1831 (PR4).
crates/common/src/bucket_stats.rs (ReplicationLatency plus a commented-out ReplicationLastMinute corpse) had zero consumers anywhere in the workspace — the live replication statistics implementation is crates/replication/src/stats.rs. LastMinuteHistogram in last_minute.rs (already carrying allow(dead_code)) was equally unreferenced, and size_to_tag / SIZE_LAST_ELEM_MARKER had no user besides the histogram, so the whole block goes with it. LastMinuteLatency and AccElem stay: common's metrics.rs uses them.
Ref rustfs/backlog#1833 (PR5).
The peer-edit delivery fence from #5882 treated an equal applied generation as stale. One edit legitimately fans out one delivery per peer record under a single generation (the ILM-expiry edit sends every peer's record), so the receiver applied only the first body, raised its high-water mark, and silently acked-success while dropping the rest — enableILMExpiryReplication never converged on receiving sites and the three-node nightly e2e failed deterministically (issue #5767).
Only a strictly newer applied generation is stale now. Equal generation implies the same logical edit and re-applying a delivery is idempotent (update_peer overwrites the peer record; the mark is raised with max), while strictly older deliveries — the cross-node ordering case the fence exists for — stay rejected.
Adds a composed unit test driving three same-generation bodies through the receiver's fenced sequence, and widens the replication e2e's two site-replication wait helpers from a 10s polling ceiling to the 30s deadline the file's other waits use.
PutBucketVersioning with Status=Suspended on a bucket that carries a replication configuration now fails with InvalidBucketState, matching AWS S3 and MinIO. Suspension would start minting null versions that the versioned replication engine can never converge — the state is unreachable on AWS and MinIO, and the nightly acceptance-matrix e2e that tried to exercise it failed every night since it landed (issue #5767).
The acceptance-matrix test tail now pins the rejection contract (InvalidBucketState) and verifies a fresh matched PUT still replicates with a real version id after the rejected suspension.
crates/io-metrics/src/config.rs was a near-copy of io-core's Backpressure/Deadlock configuration with already-drifted field names (high_watermark vs io-core's high_water_mark) and had no consumer outside the crate's own example: the canonical BackpressureConfig lives in crates/io-core/src/backpressure.rs. Delete the module, its lib.rs re-exports, and the example's unified-config section, and settle the corresponding ARCHITECTURE.md ledger line that tracked this copy's removal.
Ref rustfs/backlog#1833 (PR4).
* chore(iam): remove eight dead error variants
iam::Error mirrored policy::Error variant-for-variant, and eight of the twins had zero construction and zero match sites anywhere in the workspace: InvalidServiceType, ErrCredMalformed, CredNotInitialized, JWTError, NoAccessKey, InvalidToken, InvalidAccessKey, InvalidExpiration (each verified by repo-wide sweep; the InvalidToken hits elsewhere are KeystoneError's unrelated variant). Delete the variants along with their Clone and PartialEq arms.
The From<policy::Error> mapping keeps its exhaustive match: the eight orphaned arms now route through a grouped binding to Error::StringError(err.to_string()), so the rendered message is preserved; nothing could observe the old discriminants because no site ever matched on them.
Ref rustfs/backlog#1831 (PR1).
* fix(iam): make Error clone variant-preserving via Arc payloads
iam::Error's hand-written Clone demoted PolicyError and CryptoError to StringError because their payloads are not cloneable — a clone changed the variant identity. There is no production clone site today (the issue's refuter confirmed this is preventive hardening, not a live bug), but any future holder of a cloned error would match the wrong variant.
The two payloads are now Arc-wrapped, so Clone is a cheap reference bump that keeps the variant. Display strings are unchanged ({0} and crypto: {0}); the #[from] derives become manual From impls wrapping in Arc; the one behavioral trade-off is that source() is no longer forwarded for these two variants (Arc<E> does not implement std::error::Error), which nothing in the workspace consumed. A regression test pins discriminant and rendered message across clone for the hard-to-clone variants.
Ref rustfs/backlog#1831 (PR2).
Keep fresh metadata fanout results owned while sharing cache-backed metadata through Arc, avoiding deep clones on eligible local cache hits without enabling unsafe distributed caching.
Co-authored-by: heihutu <heihutu@gmail.com>
The shared module rustfs_utils::http::object_encryption_keys is the single source of truth for encryption metadata key names, but three call sites still carried their own copies or bare literals: crates/kms/src/service.rs (two private constants plus four bare x-rustfs-encryption-* literals on both the write and read path), rustfs/src/app/select_object.rs (six SELECT_* copies), and rustfs/src/storage/options.rs (two private prefix copies now imported from header_compat). All values are unchanged, so the change is a compiler-verified rename.
The reader-only x-rustfs-internal-server-side-encryption- family gets a named constant with the verified judgment recorded on it: no writer emits these keys anywhere in the repo (the SSE writer persists the MinIO-branded keys verbatim for interop), the two comments claiming the dual-key invariant writes this twin were wrong and are corrected, and the defensive redaction/strip readers are kept because removing them is risk-asymmetric.
rustfs-kms's rustfs-utils dependency now declares the http feature it uses instead of relying on feature unification from sibling crates.
Refs rustfs/backlog#1775, rustfs/backlog#1562.
Keep common shard-indexed decode scratch vectors inline while preserving heap fallback for larger supported erasure layouts. Consume scratch iterators directly at the stripe-state boundary to avoid reallocating.
Co-authored-by: heihutu <heihutu@gmail.com>
The nine *_missing_from_policy_conditions tests in multipart_auth_test.rs were literal-for-literal identical after normalization: policy pins bucket + key + content-length-range, the form smuggles one extra field the policy never declared, and the upload must be rejected with 403 AccessDenied naming the field. Each one started its own full server.
This adds the run_post_object_policy_case helper (parameterized by bucket, key, policy conditions, extra form fields, file body, and expected status/code/mention, with a per-case assertion prefix) and folds the nine tests into one table-driven test with nine rows. Every row keeps its original test's exact bucket, key, field name/value, body bytes, and expected error strings — including the two rows that asserted the stronger <Code>AccessDenied</Code> form — so no poison value is lost. The helper's signature is general enough for the policy_mismatch and sse-kms groups planned as PR2/PR3.
cargo nextest list now reports 103 tests for this module; the inventory row said 109 while the file actually held 111 before this change (stale by two), so the inventory is set to the measured 103 in the same diff per the issue's hard constraint.
Ref rustfs/backlog#1838 (PR1).
No workflow ever set RUSTFS_KMS_VAULT_TOKEN, so live_vault_backends() returned an empty set in every CI run and behavior_rotation.rs never asserted the working half of rotate/versioning; the #[ignore] live-Vault tests had never executed in CI either. nightly-gnu.yml gains a kms-vault-lane job (vault server -dev with KV2 + Transit, full rustfs-kms suite with the lane on, the dev-Vault ignored tests, and the AppRole live script) plus a separate kms-vault-ha-failover job for the three-node Raft failover script, isolated so an election-timing flake cannot mask the main lane's verdict. GitHub-hosted ubuntu-latest rather than the self-hosted fleet: the HA script needs Docker, and e2e-s3tests.yml's banner records how the heterogeneous sm-standard pods burned the last docker-dependent workflow.
The behavior harness now records every key TestKms::create_key mints and deletes them after each Vault-backed for_each_backend case, on a fresh manager over the same configuration with the immediate-deletion gate enabled for cleanup only. Transit needs the deletion issued twice (first call parks the key in PendingDeletion, the second destroys it); KV2 destroys on the first call. Verified against a real dev Vault: after a full suite run the server holds zero behavior-* keys.
Also fixes test_vault_cancel_key_deletion_persists_state, which was broken by construction — Default::default() never picks up the insecure-dev-defaults env override, so the HTTP dev Vault the test requires was always refused. It now declares development mode on the config, and passes.
Refs rustfs/backlog#1774, rustfs/backlog#1562.
put_object_part's commit phase held an exclusive write lock on the whole
upload_id_path namespace, so concurrent UploadPart commits for different
part numbers of one upload serialized behind a single lock and returned
503 once the 5s lock-acquire timeout elapsed.
Adopt MinIO's PutObjectPart lock scope: a shared read lock on the
uploadId namespace plus an exclusive write lock on
{upload_id_path}/part.{N}. Different part numbers now commit
concurrently; same-part retries still serialize (backlog#853);
complete/abort keep the uploadId write lock and still exclude every
in-flight part commit. The lock-loss fence covers both guards.
Fixes#5961
test_format_v1 (ecstore layout::format) only printed its results; the pinned v1 format.json literal never parsed at all because "this": null fails Uuid deserialization, and the Err was silently discarded. Fix the fixture to the real on-disk shape (MinIO and RustFS always write a concrete disk UUID there) and assert a serialize->parse roundtrip identity plus every pinned field of the literal.
test_console_cors_configuration discarded all four parse_cors_origins results; parse_cors_origins returns an opaque CorsLayer, so the test now drives real CORS preflight requests through an axum router and asserts the allow-origin outcomes: wildcard answers any origin with *, a configured list echoes listed origins and refuses unlisted ones, empty/unset configurations allow no cross-origin caller.
test_heal_channel_processor_new only constructed the processor; it now asserts the response channel accepts a send.
Ref rustfs/backlog#1836 (PR1).
kms/service.rs re-declared x-rustfs-encryption-key-id and x-rustfs-encryption-algorithm locally; the canonical owners live in rustfs-utils' object_encryption_keys module, which kms already transitively builds. Enable the http feature on the existing rustfs-utils dependency and import the two constants instead. The explanatory comment about why the algorithm header exists (SSE mode vs AEAD cipher round-trip) moves to the import site.
No dependency-graph change (cargo tree -p rustfs-kms is unchanged apart from the feature) and no behavior change: the imported values are byte-identical.
Ref rustfs/backlog#1833 (PR2).
backlog#1833 PR1 prescribed deduplicating crates/replication/src/http.rs onto the canonical rustfs-utils http modules via a re-export facade. That plan conflicts with a standing architecture guard the issue's review missed: check_architecture_migration_rules.sh rejects any rustfs-utils import or dependency from the replication crate ("replication crate HTTP/helper contracts must not import or depend on rustfs-utils"), the same way it bans rustfs-filemeta and rustfs-storage-api — the wire-contract crate deliberately has zero internal dependencies.
So this lands the issue's fallback shape instead (the same bidirectional do-not-merge pattern the issue itself prescribes for the policy path.rs cluster): a module doc on replication/http.rs naming the canonical owners and the guard that forces the local copy, mirror notes on utils' metadata_compat.rs and header_compat.rs, and a new test pinning every duplicated constant to its literal wire value so the two copies cannot drift silently.
No production code changed.
Ref rustfs/backlog#1833 (PR1).
Deletes three blocks of commented-out code that can never be revived: seven dead tokio::tests plus ~20 commented use statements at the tail of ecstore's store/list_objects.rs test module (all hardcoded to a developer's personal machine path), the commented test_extract_claims in policy/utils.rs, and the commented-out pre-strum AdminAction enum draft in policy/action.rs (the live enum below it is untouched).
Also rewords two doc comments on the bucket-metadata inline-data interop test to drop the personal attribution while keeping the technical content, so the corpse check (rg weisd) now returns zero across the repo.
Ref rustfs/backlog#1836 (PR2).
Delete five zero-caller client modules (api_bucket_policy,
api_get_object_acl, api_get_object_attributes, api_get_object_file,
api_restore), the orphaned TransitionCore get/put_bucket_policy
wrappers, and the two constants only they consumed. Also removes two
unwrap() panics on remote-controlled data in api_get_object_acl.rs
(non-UTF-8 body, missing Owner ID). Tier warm-backend APIs untouched.
Ref: rustfs/backlog#1822 (T1)
Delete the dead ecstore save_data_usage_cache and the thin
DataUsageCache::marshal_msg it was the only caller of, drop the
Serialize derive (and the dead DataUsageCacheStorage trait with its
save path) from the thin projection types so no write path can exist
outside the scanner's canonical map-encoded writer, and pin the
persisted .usage-cache.bin wire bytes with cross-crate fixture tests
on both the scanner writer and the thin reader.
Refs rustfs/backlog#1828 (T1-T3).
The StorageError -> DiskError conversion dropped seven variants with
exact DiskError counterparts (FaultyRemoteDisk, DiskAccessDenied,
DriveIsRoot, IsNotRegular, VolumeNotEmpty, VolumeAccessDenied,
FileAccessDenied) into the DiskError::other fallback, degrading them to
an opaque Io error. Two of them sit on the quorum ignore-lists in
disk/error_reduce.rs, so a degraded instance would stop matching the
ignore list and count toward the dominant error in reduce_errs.
Also mirror the StorageError-side io::Error downdrill in
From<io::Error> for DiskError: recover a StorageError boxed through
From<StorageError> for io::Error instead of wrapping it as Io.
Add a round-trip identity test over every DiskError variant
(DiskError -> StorageError -> DiskError) and a boxed-StorageError
recovery test.
* test(heal): strengthen replacement e2e evidence
* test(heal): fix replacement e2e barriers
Accept the real post-fault scanner failure-to-idle sequence as the live disk loss barrier, and preserve the first definitive completed status while only resampling the physical census for premature-completion confirmation.
Co-Authored-By: heihutu <heihutu@gmail.com>
---------
Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: heihutu <heihutu@gmail.com>
fix(get): give UringBackend the only fd cache for its disk (#1801)
#1801 made `StdBackend::new` always build a descriptor cache. `UringBackend`
wraps a `StdBackend` (`inner`), so under io_uring a disk ended up with TWO
`FdCache`s: the wrapper's and the inner's. `UringBackend::pread_bytes`
delegates to `inner.pread_bytes` on four fallback paths (latch-off, O_DIRECT
unsupported / error, buffered-read error), which populated `inner.fd_cache` —
but `UringBackend`'s invalidation only touches its own cache, so the inner
cache was never invalidated. For up to `FD_CACHE_TTL` (5s) after a heal/rename/
delete, a fallback read could serve the pre-mutation inode: exactly the
stale-descriptor hazard `FdCache`'s generation guard exists to close
(rustfs/backlog#1176). It also double-counted `FD_CACHE_CAPACITY` (512 fds)
against `RLIMIT_NOFILE` per disk (backlog#1178).
Fix: `UringBackend` now constructs its inner `StdBackend` with the new
`StdBackend::new_without_fd_cache`, so the wrapper owns the only cache for the
disk. The inner backend opens per read on fallback, leaving nothing
unguarded. `StdBackend::new` (standalone default) is unchanged; a private
`build(root, build_fd_cache)` holds the shared construction.
- Default (non-io_uring) path: byte-for-byte unchanged.
- io_uring path: one cache per disk, fully covered by the wrapper's
invalidation; halves the per-disk fd budget under `RLIMIT_NOFILE`.
- `RUSTFS_IO_URING_FD_CACHE` / `RUSTFS_LOCAL_FD_CACHE` semantics preserved.
- Regression test pins `new_without_fd_cache` -> no cache.
Found by a post-merge re-review of the Wave 1 GET PRs. cargo check/clippy
clean; Linux compile + the io_uring fd-cache suite deferred to CI.
Co-authored-by: heihutu <heihutu@gmail.com>
Every GET fans out a `read_version` across all disks to resolve xl.meta. Each
fanout allocated an `Arc<ReadOptions>` (3 bools) plus four `Arc<String>`
(`Arc::new(x.to_string())` = two allocations each) and cloned them into every
spawned task. This trims the per-fanout allocation footprint.
- `ReadOptions` is three bools, so it is now `Copy`. The fanout drops the
`Arc<ReadOptions>` and hands each spawned task a copy; the two pre-existing
`ReadOptions::clone()` sites (set_disk/read.rs, set_disk/ops/heal.rs) stop
cloning a `Copy` type.
- The four request strings use `Arc::<str>::from(&str)` (one allocation each)
instead of `Arc::new(..to_string())` (string buffer + Arc = two each) — four
fewer allocations per fanout, transparent to the `read_version(&str)` call.
Behavior is unchanged: the fanout still spawns one task per disk (the spawn is
deliberate — `read_version_call_counter_observes_spawned_fanout` verifies the
process-global counter observes every per-disk increment across workers), quorum
/ early-stop / full-wait semantics are untouched, and no result ordering or
error handling changed.
Two larger items from the audit are intentionally NOT in this PR:
- `tokio::spawn` -> `FuturesUnordered`: the spawn is a tested, deliberate
design (cross-worker counter observation for #1309/#1314), and converting
would also change panic isolation. Left as-is.
- `vec![FileInfo::default(); N]`: `FileInfo`'s empty containers (String /
HashMap / Vec) do not allocate, so this is one `Vec` allocation, not the
per-element allocation the audit implied — not a real hot spot.
`cargo fmt`, `cargo clippy -p rustfs-ecstore --lib` (0 warnings),
`cargo check --lib --tests`, and the 26 fanout / call-counter unit tests pass
on macOS (the change is fully cross-platform).
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: zhi22915 <qiuzgang@gmail.com>
A small object whose data shards are inlined in xl.meta can be reassembled
straight from the already-resolved metadata, skipping the Erasure reconstruct
pipeline. The fast path existed but was opt-in
(`RUSTFS_GET_SMALL_OBJECT_DIRECT_MEMORY`, default off), so every deployment
paid the full shard-read fan-out for eligible small GETs by default.
- Flip `DEFAULT_RUSTFS_GET_SMALL_OBJECT_DIRECT_MEMORY` to `true`. The path is
correctness-neutral on a miss: `try_get_object_direct_data_shards_*` returns
None when the inline reassembly cannot satisfy the read, and the GET then
proceeds through the normal shard-read pipeline. The env stays as a kill
switch (`=false` restores the legacy path).
- Drop the bucket-level `versioned` / `version_suspended` exclusions. The
decision is made on `fi` — the already-resolved target version — so
reassembling its inlined data is correct on a versioned bucket too. An
explicit versionId GET still falls back (`opts.version_id`), and a
delete-marker latest is still rejected. The two now-unused fallback reasons
and their metric labels are removed.
- Tests updated: a versioned latest-version object is now eligible / `Use`
(covers the newly allowed path); the removed reasons' label assertions are
dropped.
cargo check --lib --tests and the direct_memory unit tests pass on macOS
(the change is fully cross-platform).
Co-authored-by: heihutu <heihutu@gmail.com>
StdBackend opened, stat'd, and access-checked the shard file on every
positioned read, so each small-object GET paid N x (open + access + fstat)
syscalls even for hot shards. UringBackend already caches descriptors
behind a generation-guarded, rlimit-bounded moka cache with full
rename/delete/heal invalidation; StdBackend had no equivalent.
Port that cache to the default backend:
- StdBackend gains an `fd_cache: Option<FdCache>` (Linux only, mirroring
UringBackend), built in `new()` behind `RUSTFS_LOCAL_FD_CACHE` (default
on) and the same `rlimit_allows_fd_cache` guard.
- pread_bytes consults the cache on the buffered path: a hit reuses the
descriptor via `dup` (one syscall, no path resolution or permission
re-check) and skips volume access; a miss opens as before, snapshots the
invalidation generation, and hands the freshly opened descriptor back
for `insert_if_fresh`, which refuses to cache if a heal/delete bumped the
generation mid-open (rustfs/backlog#1176). O_DIRECT reads keep opening
their own aligned descriptors.
- The DirectReadCopy branch switches from seek+read_exact to
`FileExt::read_exact_at`: a `dup`'d cached descriptor shares the source
descriptor's open-file offset, so a positioned read (like the mmap path's
offset argument) keeps concurrent cache hits on the same shard correct.
- The four `LocalIoBackend` invalidation methods now drop stale entries on
StdBackend. LocalDisk already calls them on rename_data/rename_file/
delete/delete_volume/close, so no new call sites are needed.
- Two tests mirror the io_uring ones: a heal rename must be hidden until
invalidate_cached_fds_under runs, and a repeated read caches exactly one
descriptor that prefix invalidation drops.
Behavior is byte-for-byte unchanged on a miss and on non-Linux; the cache
is auto-disabled only when RLIMIT_NOFILE is too low. macOS cargo check
--lib and --tests pass; Linux compile deferred to CI.
Co-authored-by: heihutu <heihutu@gmail.com>
Treat recovery-directory lookup on a replacement endpoint already deferred as replacement_path_unavailable as an expected debug diagnostic instead of a durable generation conflict.
Keep real survivor recovery conflicts and corrupt records on the existing warning/blocking path.
Co-authored-by: heihutu <heihutu@gmail.com>
* test(heal): add privileged replacement rebuild e2e
Add ignored Linux-only 3x4 automatic replacement coverage for EC8+4 and EC6+6. The tests use real tmpfs mounts in an isolated mount namespace, wait for scanner-driven replacement recovery status, and verify the replacement target with per-version xl.meta and part.N physical census without invoking Admin deep heal.
Co-Authored-By: heihutu <heihutu@gmail.com>
* fix(test): avoid unsafe in privileged replacement e2e
Co-Authored-By: heihutu <heihutu@gmail.com>
* test(heal): harden privileged replacement e2e
Co-Authored-By: heihutu <heihutu@gmail.com>
* test(heal): prove absent replacement recovery witness
Co-Authored-By: heihutu <heihutu@gmail.com>
* test(e2e): prove absent replacement observation
Stop the target node before detaching the test mount so RustFS releases its mount lease instead of continuing to serve the old tmpfs through an open fd. Restart the node with the endpoint absent and wait for the scanner's real readiness rejection in that node's log.
Assert the absent window has no replacement intent, completion proof, checkpoint, healing marker, or Admin v4 durable record for the target before mounting the blank replacement and waiting for automatic recovery plus physical shard census.
Co-Authored-By: heihutu <heihutu@gmail.com>
* test(e2e): streamline cluster log capture
Move cluster-node log capture out of ClusterNode and into per-node cluster launch configuration so the privileged replacement E2E uses an explicit harness API instead of mutating node identity data.
Reuse the same stdout/stderr capture helper for single-node and cluster processes, and pin the per-node capture behavior with a focused common test.
Co-Authored-By: heihutu <heihutu@gmail.com>
* test(e2e): harden privileged replacement proof
Co-Authored-By: heihutu <heihutu@gmail.com>
---------
Co-authored-by: heihutu <heihutu@gmail.com>
* test(site-replication): pin retry-event lost-update against locked RMW (red)
P1-15 (rustfs/backlog#1675 B2): the site-replication retry-event writers
(enqueue/dequeue, which hang off every hook broadcast path) perform a
load -> mutate -> persist without taking SITE_REPLICATION_STATE_LOCK, so
a single process can lose a concurrent lock-holding writer's update; the
service-side reload path is equally unlocked, and no writer holds a
distributed lock across the read-modify-write, so multi-node RMW loses
updates even where the process lock is held.
Red evidence (current main): replaying enqueue's exact three steps around
a completed mark_pending_rotation_peer_acked commit wipes the rotation
ack — the final state holds the retry event but not the ack.
* fix(site-replication): route state RMW through one locked transaction
P1-15 PR1 (rustfs/backlog#1675 B2). The site-replication state object
(config/site-replication/state.json, which also carries the retry-event
queue) was mutated through read-modify-write sequences with inconsistent
locking: the retry-event writers on every hook broadcast path and the
RPC-driven service reload took no lock at all (single-process lost
updates, pinned by the red commit), and no writer held a distributed lock
across the whole RMW (cross-node lost updates everywhere).
- New admin/site_replication_state module: the state transaction boundary
`with_site_replication_state_lock[_on]` — process mutex plus the
distributed config-object write lock (the pattern proven by the repair
state), with the shared path constant. The process mutex is transitional
until PR2 migrates the remaining ~26 call sites.
- handlers: typed `update_site_replication_state` (no-lock load /
persist-or-clear inside the boundary; normalizes the peer map exactly
once, retiring the double-clone/double-normalize persist path, P2-22).
Migrated: retry-event enqueue (always-write), dequeue (lock-free probe,
transaction on hit), mark_pending_rotation/remove_peer_acked.
- service reload: the tolerant byte-level read->normalize->save now runs
inside the same boundary via no-lock IO — a cluster-wide reload fan-out
can no longer overwrite a concurrent state writer. Normalization
semantics untouched (all six service-side tests unchanged and green).
- Add/PeerJoin/Edit handlers release the state guard before their peer
fan-out: the transport helpers' retry-event bookkeeping now re-enters
the state transaction and must not nest inside the guard (the
adversarial review caught this as a re-entrancy deadlock; the fix
mirrors the Remove/Rotate handlers' existing scope). The Edit non-
refresh branch commits before fanning out — the old fanout-first order
recorded retry events pointing at a state the local site had not saved.
- ecstore: delete_config_no_lock (+ facade/bridge exports) so the clear
half of persist-or-clear works under the held object lock.
Red -> green: the red commit pinned the deterministic lost-update
interleaving (stale retry-event persist wiping a committed rotation ack);
the test now drives the real functions concurrently for 8 rounds and
asserts every retry event and every ack survives. Full
handlers/service site-replication unit suites green (171 + 6); dual-node
site-replication e2e (state edit fresh/stale, object replication) green;
fmt / clippy / logging guardrails clean.
Adversarial review: one blocking finding (the re-entrancy deadlock above)
fixed and re-verified by a full second pass over all 30 lock sites and
the Add/Join/Edit call graphs. Non-blocking notes recorded for PR2:
mark_* now persists on miss (persist-or-clear semantics; a miss-skip
return is a cheap follow-up), Add still holds the guard across the peer
join probe (pre-existing availability debt), and a timeout-guarded
unreachable-peer regression test for the fan-out paths.
* fix(site-replication): keep the state mutex behind an owner helper
CI's architecture migration guard lists SITE_REPLICATION_STATE_LOCK as an
owner-local static, so it may not be `pub(crate)`. Keep it private to the
new module and let the not-yet-migrated RMW call sites take it through
`site_replication_state_process_guard()` — the sanctioned owner-helper
pattern; the helper disappears with the mutex in PR2.
* fix(site-replication): keep peer-edit delivery under the state guard
Review follow-up (#5882).
Releasing the guard before the fan-out (my deadlock fix) traded the
ordering the guard used to provide: edit A could commit and stall while
edit B committed and reached a peer first, then A arrived last and won.
The peer edit handler applies whatever arrives — it has no generation or
updated-at fence — and a successful stale delivery is not repaired by the
retry queue, so the sites diverge silently.
The fan-out is back under the guard. What actually could not run there is
the retry-event bookkeeping, which re-enters the state transaction, so the
edit branch now delivers with the plain transport and settles the retry
queue after the guard is released: successes dequeue, the first failure
enqueues and is returned. Ordering and bookkeeping both preserved. The add
handler keeps its peer-edit finalize fan-out under the guard for the same
reason and releases only before bootstrap/back-fill, which send bucket-ops
(not peer edits) through retry-event transports.
The concurrency test could not tell the two guards apart — both writers
took both locks, so it passed with either removed. Replaced by two tests
that isolate one guard each, both verified by mutation:
- a process-only legacy writer (the shape the not-yet-migrated call sites
still use) racing the transaction: fails when the transaction stops
taking the process mutex;
- two writers that bypass the process mutex, as separate nodes do, driving
the production object-lock path (`with_site_replication_state_object_lock`
factored out for exactly this): fails when the distributed lock is
removed.
Verification: handlers 173 + service 6 unit tests green; site-replication
dual-node and three-node edit e2e green; arch/layer/logging guardrails,
fmt and clippy clean.
* fix(site-replication): fence peer-edit delivery by generation
Review follow-up on the two remaining holes in the edit path.
Ordering was only process-local. `SITE_REPLICATION_STATE_LOCK` is per
node, so holding it across the fan-out orders the edits ONE node accepts
and nothing else: two nodes of the same site can both commit and reach a
peer in the opposite order, and the peer edit handler applied whatever
arrived last. Each edit now takes a generation from
`SiteReplicationState::edit_generation`, allocated in the same commit as
the edit itself — i.e. under the distributed state-object lock, so two
nodes can never share one. The generation rides the peer-edit request as
query parameters and the receiver rejects (acks without applying) a
delivery at or below the mark it already applied for that origin site,
recording the mark in the same commit as the edit it fences. Peers that
predate the fence send no parameters and are applied as before.
Retry settlement could discard a newer failure. After the guard is
released, a success for edit A removed every retry event for
(peer, peer-edit): if edit B committed, failed its own delivery and
enqueued while A was in flight, A erased it — local state B, peer on A,
nothing queued to converge them. Settlement now only removes events whose
recorded generation is not newer than the one being settled, and a later
failure never lowers the fence. Broadcast paths carry no generation and
settle unconditionally as before; their events live under their own
paths and cannot collide with a peer-edit delivery.
A departed peer's mark is dropped on load: a site that leaves drops below
two peers, which clears its state object and restarts its counter at
zero, so a leftover mark would reject every edit it sends after it
rejoins.
Tests: two-node generation uniqueness (drop the object lock and the two
nodes collide), the receiver's staleness predicate and its wiring, the
settlement interleaving (drop the fence and B's retry is erased), and the
rejoin reset.
Refs: rustfs/backlog#1675 (P1-15)
Zhengchao An