From f99956eade54372336cbd4c631e246aa019cb045 Mon Sep 17 00:00:00 2001 From: houseme Date: Wed, 29 Jul 2026 09:21:23 +0800 Subject: [PATCH 1/8] test(lifecycle): classify mixed rollout harness results (#5384) * test(lifecycle): classify mixed rollout harness results Classify Docker #1508 evidence as strict, baseline, blocked, or failed so tiered-storage baseline runs cannot be mistaken for strict mixed-version rollout closure evidence. Co-Authored-By: heihutu * test: classify Docker manual transition preemption Co-Authored-By: heihutu --------- Co-authored-by: heihutu --- scripts/README.md | 2 +- ...transition_mixed_version_docker_harness.sh | 268 ++++++++++++++++-- scripts/test_manual_transition_runbooks.sh | 9 + 3 files changed, 257 insertions(+), 22 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index c88f4b679..1885201f8 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -90,7 +90,7 @@ their issue closes. | `manual_transition_journal_audit.sh` | dev-tool | Journal + metrics + log audit for manual transition jobs | — | | `manual_transition_mixed_rollout_matrix.sh` | dev-tool | Matrix generator for mixed-version rollout phases | — | | `manual_transition_mixed_rollout_runbook.sh` | dev-tool | Reusable mixed-version rollout runbook generator (external run) | — | -| `manual_transition_mixed_version_docker_harness.sh` | dev-tool | Dedicated #1508 Docker harness for old/new manual-transition rollout evidence | `test_manual_transition_runbooks.sh` | +| `manual_transition_mixed_version_docker_harness.sh` | dev-tool | Dedicated #1508 Docker harness for old/new manual-transition rollout evidence with strict/baseline/blocked result classification | `test_manual_transition_runbooks.sh` | | `monitor_manual_transition_ci.sh` | dev-tool | CI workflow/status watcher for manual transition follow-up monitoring | — | | `manual_transition_soak_matrix.sh` | dev-tool | Matrix generator for nightly stress windows | — | | `manual_transition_nightly_stress_runbook.sh` | dev-tool | Nightly stress entrypoint with failure snapshot templates | — | diff --git a/scripts/manual_transition_mixed_version_docker_harness.sh b/scripts/manual_transition_mixed_version_docker_harness.sh index 0937703fc..68a611611 100755 --- a/scripts/manual_transition_mixed_version_docker_harness.sh +++ b/scripts/manual_transition_mixed_version_docker_harness.sh @@ -25,9 +25,13 @@ COLD_IMAGE="${COLD_IMAGE:-${NEW_IMAGE}}" BASE_PORT="${BASE_PORT:-19400}" OUT_DIR="${OUT_DIR:-${PROJECT_ROOT}/target/manual-transition-1508-docker/$(date +%Y%m%dT%H%M%S)}" KEEP_UP=false -ROLLBACK_NEW2_TO_OLD=true +ROLLBACK_PHASE="${ROLLBACK_PHASE:-after-terminal}" +OLD_NODE_PHASE="${OLD_NODE_PHASE:-initial}" WAIT_TIMEOUT_SECS="${WAIT_TIMEOUT_SECS:-180}" POLL_SECONDS="${POLL_SECONDS:-180}" +TRANSITION_WORKERS="${TRANSITION_WORKERS:-2}" +TRANSITION_QUEUE_CAPACITY="${TRANSITION_QUEUE_CAPACITY:-64}" +FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT="${FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT:-false}" HOT_ACCESS_KEY="${HOT_ACCESS_KEY:-mvadmin}" HOT_SECRET_KEY="${HOT_SECRET_KEY:-mvsecret}" @@ -58,18 +62,31 @@ Options: --object-count Non-empty probe object count --tier Remote tier name --keep-up Leave Docker services running - --no-rollback Do not replace node2 with old image after job admission + --rollback-phase Rollback node2 timing: after-terminal, in-flight, none + --old-node-phase Old node1 timing: initial, before-job + --no-rollback Alias for --rollback-phase none -h, --help Show help Environment: PROJECT_NAME OLD_IMAGE NEW_IMAGE COLD_IMAGE BASE_PORT OUT_DIR KEEP_UP HOT_ACCESS_KEY HOT_SECRET_KEY COLD_ACCESS_KEY COLD_SECRET_KEY TIER_NAME TIER_BUCKET TIER_PREFIX JOB_BUCKET JOB_PREFIX OBJECT_COUNT - WAIT_TIMEOUT_SECS POLL_SECONDS AWS_SIGV4_SCOPE + ROLLBACK_PHASE WAIT_TIMEOUT_SECS POLL_SECONDS TRANSITION_WORKERS + TRANSITION_QUEUE_CAPACITY OLD_NODE_PHASE FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT AWS_SIGV4_SCOPE Artifacts: compose.yml, image inspect files, health/readiness logs, API responses, terminal status, old-node readback, container logs, summary.env. + +Result classifications: + strict_mixed_rollout_pass Real old/new images, non-empty completed transition, zero failures + baseline_tiered_storage_pass Same old/new image completed transition; useful baseline, not #1508 closure + blocked_manual_api_not_implemented Manual transition API returned 501 before job admission + blocked_manual_api_unavailable Manual transition API did not return a usable job_id + blocked_cluster_readiness_failed Docker cluster did not reach health/readiness before admission + blocked_empty_scan_or_lifecycle Job completed without lifecycle-matching transition work + blocked_manual_job_preempted_by_lifecycle_queue Lifecycle/immediate transition queued work before the job + strict_mixed_rollout_fail Mixed rollout ran but did not satisfy the strict #1508 gate USAGE } @@ -111,6 +128,28 @@ parse_positive_int() { fi } +validate_rollback_phase() { + case "$ROLLBACK_PHASE" in + after-terminal|in-flight|none) + ;; + *) + log_error "--rollback-phase must be one of: after-terminal, in-flight, none" + exit 1 + ;; + esac +} + +validate_old_node_phase() { + case "$OLD_NODE_PHASE" in + initial|before-job) + ;; + *) + log_error "--old-node-phase must be one of: initial, before-job" + exit 1 + ;; + esac +} + parse_args() { while [[ $# -gt 0 ]]; do case "$1" in @@ -150,8 +189,16 @@ parse_args() { KEEP_UP=true shift ;; + --rollback-phase) + ROLLBACK_PHASE="$(arg_value "$1" "${2:-}")" + shift 2 + ;; + --old-node-phase) + OLD_NODE_PHASE="$(arg_value "$1" "${2:-}")" + shift 2 + ;; --no-rollback) - ROLLBACK_NEW2_TO_OLD=false + ROLLBACK_PHASE=none shift ;; -h|--help) @@ -198,12 +245,18 @@ cleanup() { } write_compose_file() { + local node1_image COMPOSE_FILE="${OUT_DIR}/compose.yml" + node1_image="$OLD_IMAGE" + if [[ "$OLD_NODE_PHASE" == "before-job" ]]; then + node1_image="$NEW_IMAGE" + fi cat >"$COMPOSE_FILE" <"${OUT_DIR}/run-response.json" + curl_hot POST "$(hot_endpoint 2)/rustfs/admin/v3/ilm/transition/run?${query}" \ + -o "${OUT_DIR}/run-response.json" \ + -w "%{http_code}\n" >"${OUT_DIR}/run-response.http_code" || true + response="$(cat "${OUT_DIR}/run-response.json" 2>/dev/null || true)" + run_http_code="$(cat "${OUT_DIR}/run-response.http_code" 2>/dev/null || true)" job_id="$(printf '%s' "$response" | jq -r '.job_id // empty')" if [[ -z "$job_id" ]]; then - log_error "manual transition response omitted job_id" + log_warn "manual transition response omitted job_id, http_code=${run_http_code:-unknown}" return 1 fi printf '%s\n' "$job_id" >"${OUT_DIR}/job-id.txt" @@ -451,19 +516,25 @@ start_transition_job() { replace_node2_with_old_image() { local network network="$(network_name)" - log_info "Replacing node2 with old image ${OLD_IMAGE} for in-flight rollback readback" + log_info "Replacing node2 with old image ${OLD_IMAGE} for ${ROLLBACK_PHASE} rollback readback" compose stop node2 >/dev/null compose rm -f node2 >/dev/null docker run -d \ --name "${PROJECT_NAME}-node2-rollback-old" \ --network "$network" \ --network-alias node2 \ + --user 0:0 \ -p "$((BASE_PORT + 2)):9000" \ -e RUSTFS_ADDRESS=:9000 \ -e RUSTFS_ACCESS_KEY="$HOT_ACCESS_KEY" \ -e RUSTFS_SECRET_KEY="$HOT_SECRET_KEY" \ -e RUSTFS_VOLUMES="$(hot_volumes)" \ -e RUSTFS_SCANNER_ENABLED=false \ + -e RUSTFS_SCANNER_CYCLE=3600 \ + -e RUSTFS_SCANNER_START_DELAY_SECS=3600 \ + -e RUSTFS_MAX_TRANSITION_WORKERS="$TRANSITION_WORKERS" \ + -e RUSTFS_TRANSITION_QUEUE_CAPACITY="$TRANSITION_QUEUE_CAPACITY" \ + -e RUSTFS_TEST_FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT="$FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT" \ -e RUSTFS_UNSAFE_BYPASS_DISK_CHECK=true \ -e RUSTFS_OBS_LOGGER_LEVEL=warn \ -v "${PROJECT_NAME}_node2_data_0:/data/rustfs0" \ @@ -473,6 +544,37 @@ replace_node2_with_old_image() { "$OLD_IMAGE" >/dev/null } +replace_node1_with_old_image() { + local network + network="$(network_name)" + log_info "Replacing node1 with old image ${OLD_IMAGE} before manual transition job" + compose stop node1 >/dev/null + compose rm -f node1 >/dev/null + docker run -d \ + --name "${PROJECT_NAME}-node1-before-job-old" \ + --network "$network" \ + --network-alias node1 \ + --user 0:0 \ + -p "$((BASE_PORT + 1)):9000" \ + -e RUSTFS_ADDRESS=:9000 \ + -e RUSTFS_ACCESS_KEY="$HOT_ACCESS_KEY" \ + -e RUSTFS_SECRET_KEY="$HOT_SECRET_KEY" \ + -e RUSTFS_VOLUMES="$(hot_volumes)" \ + -e RUSTFS_SCANNER_ENABLED=false \ + -e RUSTFS_SCANNER_CYCLE=3600 \ + -e RUSTFS_SCANNER_START_DELAY_SECS=3600 \ + -e RUSTFS_MAX_TRANSITION_WORKERS="$TRANSITION_WORKERS" \ + -e RUSTFS_TRANSITION_QUEUE_CAPACITY="$TRANSITION_QUEUE_CAPACITY" \ + -e RUSTFS_TEST_FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT="$FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT" \ + -e RUSTFS_UNSAFE_BYPASS_DISK_CHECK=true \ + -e RUSTFS_OBS_LOGGER_LEVEL=warn \ + -v "${PROJECT_NAME}_node1_data_0:/data/rustfs0" \ + -v "${PROJECT_NAME}_node1_data_1:/data/rustfs1" \ + -v "${PROJECT_NAME}_node1_data_2:/data/rustfs2" \ + -v "${PROJECT_NAME}_node1_data_3:/data/rustfs3" \ + "$OLD_IMAGE" >/dev/null +} + poll_terminal_status() { local job_id="$1" local status_url status_json terminal_state @@ -512,6 +614,85 @@ head_probe() { -w "%{http_code}\n" >"${OUT_DIR}/head-object.http_code" || true } +image_id() { + local file="$1" + jq -r '.[0].Id // ""' "$file" 2>/dev/null || true +} + +is_compat_readback_code() { + case "$1" in + 200|501) + return 0 + ;; + *) + return 1 + ;; + esac +} + +classify_result() { + local terminal_state="$1" + local transition_completed="$2" + local transition_failed="$3" + local tier_failure="$4" + local old_code="$5" + local rollback_code="$6" + local run_http_code="$7" + local lifecycle_config_found="$8" + local scanned="$9" + local eligible="${10}" + local skipped_already_transitioned="${11}" + local skipped_already_in_flight="${12}" + local old_image_id new_image_id images_are_mixed readback_ok + + if [[ -f "${OUT_DIR}/readiness-failed" ]]; then + printf 'blocked_cluster_readiness_failed\n' + return + fi + + old_image_id="$(image_id "${OUT_DIR}/old-image.inspect.json")" + new_image_id="$(image_id "${OUT_DIR}/new-image.inspect.json")" + images_are_mixed=false + if [[ "$OLD_IMAGE" != "$NEW_IMAGE" && -n "$old_image_id" && -n "$new_image_id" && "$old_image_id" != "$new_image_id" ]]; then + images_are_mixed=true + fi + + readback_ok=false + if is_compat_readback_code "$old_code"; then + if [[ "$ROLLBACK_PHASE" == "none" ]] || is_compat_readback_code "$rollback_code"; then + readback_ok=true + fi + fi + + if [[ "$run_http_code" == "501" ]]; then + printf 'blocked_manual_api_not_implemented\n' + return + fi + if [[ -z "$(cat "${OUT_DIR}/job-id.txt" 2>/dev/null || true)" ]]; then + printf 'blocked_manual_api_unavailable\n' + return + fi + if [[ "$terminal_state" == "completed" && ( "$lifecycle_config_found" != "true" || "$scanned" == "0" || "$eligible" == "0" || "$transition_completed" == "0" ) ]]; then + printf 'blocked_empty_scan_or_lifecycle\n' + return + fi + if [[ "$transition_completed" == "0" && "$eligible" != "0" && "$transition_failed" == "0" && "$tier_failure" == "0" ]]; then + if [[ "$skipped_already_transitioned" != "0" || "$skipped_already_in_flight" != "0" ]]; then + printf 'blocked_manual_job_preempted_by_lifecycle_queue\n' + return + fi + fi + if [[ "$terminal_state" == "completed" && "$transition_completed" != "0" && "$transition_failed" == "0" && "$tier_failure" == "0" ]]; then + if [[ "$images_are_mixed" == "true" && "$readback_ok" == "true" ]]; then + printf 'strict_mixed_rollout_pass\n' + return + fi + printf 'baseline_tiered_storage_pass\n' + return + fi + printf 'strict_mixed_rollout_fail\n' +} + collect_logs() { local service if [[ -z "$COMPOSE_FILE" || ! -f "$COMPOSE_FILE" ]]; then @@ -520,37 +701,63 @@ collect_logs() { for service in cold node1 node2 node3 node4; do compose logs --no-color "$service" >"${OUT_DIR}/${service}.log" 2>/dev/null || true done + docker logs "${PROJECT_NAME}-node1-before-job-old" >"${OUT_DIR}/node1-before-job-old.log" 2>/dev/null || true docker logs "${PROJECT_NAME}-node2-rollback-old" >"${OUT_DIR}/node2-rollback-old.log" 2>/dev/null || true } summarize() { - local terminal_state transition_completed tier_failure transition_failed old_code rollback_code + local terminal_state transition_completed tier_failure transition_failed old_code rollback_code run_http_code lifecycle_config_found scanned eligible skipped_already_transitioned skipped_already_in_flight queue_queued queue_active result_classification terminal_state="$(cat "${OUT_DIR}/terminal-state.txt" 2>/dev/null || true)" transition_completed="$(jq -r '.report.transition_completed // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" tier_failure="$(jq -r '.report.tier_failure // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" transition_failed="$(jq -r '.report.transition_failed // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" old_code="$(cat "${OUT_DIR}/old-node-status.http_code" 2>/dev/null || true)" rollback_code="$(cat "${OUT_DIR}/rollback-node2-status.http_code" 2>/dev/null || true)" + run_http_code="$(cat "${OUT_DIR}/run-response.http_code" 2>/dev/null || true)" + lifecycle_config_found="$(jq -r '.report.lifecycle_config_found // false' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf 'false')" + scanned="$(jq -r '.report.scanned // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + eligible="$(jq -r '.report.eligible // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + skipped_already_transitioned="$(jq -r '.report.skipped_already_transitioned // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + skipped_already_in_flight="$(jq -r '.report.skipped_already_in_flight // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + queue_queued="$(jq -r '.queue_snapshot.queued // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + queue_active="$(jq -r '.queue_snapshot.active // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + result_classification="$(classify_result "$terminal_state" "$transition_completed" "$transition_failed" "$tier_failure" "$old_code" "$rollback_code" "$run_http_code" "$lifecycle_config_found" "$scanned" "$eligible" "$skipped_already_transitioned" "$skipped_already_in_flight")" cat >"${OUT_DIR}/summary.env" <"${OUT_DIR}/cold-image.inspect.json" compose up -d - wait_cluster_ready + if ! wait_cluster_ready; then + printf 'cluster readiness failed before manual transition admission\n' >"${OUT_DIR}/readiness-failed" + collect_logs + summarize + return 1 + fi curl_cold PUT "$(cold_endpoint)/${TIER_BUCKET}" -o "${OUT_DIR}/create-cold-bucket.response" -w "%{http_code}\n" >"${OUT_DIR}/create-cold-bucket.http_code" create_bucket "$(hot_endpoint 2)" "$JOB_BUCKET" "${OUT_DIR}/create-hot-bucket" add_tier - put_lifecycle seed_objects - start_transition_job - if [[ "$ROLLBACK_NEW2_TO_OLD" == "true" ]]; then - replace_node2_with_old_image + put_lifecycle + if [[ "$OLD_NODE_PHASE" == "before-job" ]]; then + replace_node1_with_old_image + wait_http_ok "$(hot_endpoint 1)/health" "node1-old-live" || true + wait_http_ok "$(hot_endpoint 1)/health/ready" "node1-old-ready" || true + fi + if start_transition_job; then + if [[ "$ROLLBACK_PHASE" == "in-flight" ]]; then + replace_node2_with_old_image + fi + poll_terminal_status "$(cat "${OUT_DIR}/job-id.txt")" || true + if [[ "$ROLLBACK_PHASE" == "after-terminal" ]]; then + replace_node2_with_old_image + wait_http_ok "$(hot_endpoint 2)/health" "rollback-node2-live" || true + fi + capture_old_node_readback "$(cat "${OUT_DIR}/job-id.txt")" + head_probe + else + log_warn "Skipping terminal polling because no manual transition job was admitted" fi - poll_terminal_status "$(cat "${OUT_DIR}/job-id.txt")" - capture_old_node_readback "$(cat "${OUT_DIR}/job-id.txt")" - head_probe collect_logs summarize } diff --git a/scripts/test_manual_transition_runbooks.sh b/scripts/test_manual_transition_runbooks.sh index 248a72ef3..9ad09909a 100755 --- a/scripts/test_manual_transition_runbooks.sh +++ b/scripts/test_manual_transition_runbooks.sh @@ -208,7 +208,16 @@ bash "$MIXED_DOCKER_HARNESS" --help >/tmp/manual_transition_mixed_version_docker rg -q "mixed_version_docker_harness" /tmp/manual_transition_mixed_version_docker_harness.help rg -q -- "--old-image" /tmp/manual_transition_mixed_version_docker_harness.help rg -q -- "--new-image" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q -- "--rollback-phase" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q -- "--old-node-phase" /tmp/manual_transition_mixed_version_docker_harness.help rg -q -- "--no-rollback" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "strict_mixed_rollout_pass" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "baseline_tiered_storage_pass" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "blocked_manual_api_not_implemented" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "blocked_cluster_readiness_failed" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "blocked_manual_job_preempted_by_lifecycle_queue" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "OLD_NODE_PHASE" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT" /tmp/manual_transition_mixed_version_docker_harness.help if bash "$FAILURE_SAMPLES" --endpoint http://127.0.0.1:9000 --sample >/tmp/manual_transition_failure_samples.err 2>&1; then echo "failure samples script should fail when --sample has no value" >&2 exit 1 From 5af56cbb02a09026f4a37b58d3d81dc65fb1c32e Mon Sep 17 00:00:00 2001 From: houseme Date: Wed, 29 Jul 2026 09:37:29 +0800 Subject: [PATCH 2/8] test(ci): stabilize lifecycle timeout coverage (#5404) Co-authored-by: heihutu --- .config/nextest.toml | 23 +++--- .../bucket/lifecycle/bucket_lifecycle_ops.rs | 74 +++++++----------- crates/ecstore/src/set_disk/mod.rs | 2 + crates/ecstore/src/set_disk/ops/multipart.rs | 18 +++-- crates/ecstore/src/store/object.rs | 8 +- .../src/app/lifecycle_transition_api_test.rs | 78 +------------------ 6 files changed, 61 insertions(+), 142 deletions(-) diff --git a/.config/nextest.toml b/.config/nextest.toml index 16418d2e6..0f1917bda 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -1,17 +1,14 @@ # nextest configuration for RustFS. # -# Serialize two known load-sensitive / global-state-sharing ecstore test groups -# so the full parallel nextest suite stops producing spurious failures -# (backlog #937). These tests pass in isolation but flake under the loaded -# parallel run for two distinct reasons: +# Serialize the ecstore tests that share the process-wide disk registry or +# exercise a multi-disk commit handoff across nextest process boundaries. # # * store::bucket::tests::bucket_delete_* share process/global state (disk # registry, lock client) and race make_bucket into InsufficientWriteQuorum # when run concurrently with other ecstore tests. # * bucket_lifecycle_ops::tests::concurrent_resend_same_part_commits_one_generation -# asserts a lock-acquire correctness property whose serialized cross-disk -# commits exceed the (already max'd, 60s) acquire deadline only when the -# suite saturates disk I/O. +# uses the shared multipart fixture and a deterministic uploadId-lock +# handoff, so it must not overlap another process mutating that fixture. # # serial_test's #[serial] attribute does NOT serialize these across runs: # nextest executes each test in its own process, where the in-process @@ -100,13 +97,6 @@ path = "junit.xml" # profile's own overrides list, not the default profile's). # =========================================================================== -# QUARANTINE: OPEN backlog#937 — concurrent_resend lock-acquire deadline flakes -# under saturated disk I/O in the full parallel suite. -[[profile.ci.overrides]] -filter = 'package(rustfs-ecstore) & test(concurrent_resend_same_part_commits_one_generation)' -test-group = 'ecstore-serial-flaky' -retries = 2 - # QUARANTINE: OPEN backlog#937 — store::bucket::tests::bucket_delete_* race # make_bucket into InsufficientWriteQuorum via shared global state under load. [[profile.ci.overrides]] @@ -114,6 +104,11 @@ filter = 'package(rustfs-ecstore) & test(/^store::bucket::tests::bucket_delete_( test-group = 'ecstore-serial-flaky' retries = 2 +# Keep the deterministic multipart handoff isolated across nextest processes. +[[profile.ci.overrides]] +filter = 'package(rustfs-ecstore) & test(concurrent_resend_same_part_commits_one_generation)' +test-group = 'ecstore-serial-flaky' + # QUARANTINE: OPEN rustfs#4690 — walk_dir stall-budget accounting test depends # on producer/consumer timing windows that stretch past the budget on loaded # CI runners (regression test for rustfs#4644; failed on a zero-Rust-diff PR). diff --git a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs index 901f4ae50..9033fe275 100644 --- a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs +++ b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs @@ -11112,6 +11112,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn concurrent_resend_same_part_commits_one_generation() { + use crate::set_disk::{MultipartCommitBarrier, MultipartCommitPause}; use crate::storage_api_contracts::object::ObjectIO as _; let (_paths, ecstore) = setup_test_env().await; @@ -11133,51 +11134,36 @@ mod tests { }) .collect(); - // Two independent causes can produce a spurious lock-acquire timeout - // here, and both must stay covered: - // 1. A lost/stolen fast-lock wakeup could strand a waiter until the - // deadline — fixed for real in fast_lock::shard by bounding each - // notification wait (NOTIFY_WAIT_CAP re-polling). - // 2. Under the full nextest suite on loaded CI disks, the - // *legitimately serialized* cross-disk commits can exceed the - // acquire deadline all by themselves — observed on CI at the 5s - // default and the 30s production default with six resends, and - // again at 60s, which is a hard ceiling: fast_lock clamps every - // requested timeout to MAX_ACQUIRE_TIMEOUT (60s), so raising the - // env override higher is a no-op (the Timeout error still reports - // the requested value). Keep the guard about the correctness - // property, not disk latency: request the full 60s ceiling and cap - // the queue depth at three resends, so the last waiter sits behind - // at most two serialized commits (~12s each on the slowest observed - // CI runner, comfortably inside the deadline). Three concurrent - // resends still race the streaming phase and contend on the commit - // lock, which is all the generation-mixing regression needs. - // `#[serial]` keeps the process-wide env override isolated. - let results = temp_env::async_with_vars([(rustfs_config::ENV_OBJECT_LOCK_ACQUIRE_TIMEOUT, Some("60"))], async { - let mut tasks = tokio::task::JoinSet::new(); - for payload in candidates.iter().cloned() { - let store = ecstore.clone(); - let bucket = bucket.clone(); - let upload_id = upload.upload_id.clone(); - tasks.spawn(async move { - let mut data = PutObjReader::from_vec(payload.clone()); - store - .put_object_part(&bucket, object, &upload_id, 1, &mut data, &ObjectOptions::default()) - .await - .map(|info| (info, payload)) - }); - } + let commit_barrier = MultipartCommitBarrier::install(&bucket, object, MultipartCommitPause::PutPartBeforeLockLost); + let start = Arc::new(tokio::sync::Barrier::new(candidates.len() + 1)); + let mut tasks = tokio::task::JoinSet::new(); + for payload in candidates.iter().cloned() { + let store = ecstore.clone(); + let bucket = bucket.clone(); + let upload_id = upload.upload_id.clone(); + let start = Arc::clone(&start); + tasks.spawn(async move { + start.wait().await; + let mut data = PutObjReader::from_vec(payload.clone()); + store + .put_object_part(&bucket, object, &upload_id, 1, &mut data, &ObjectOptions::default()) + .await + .map(|info| (info, payload)) + }); + } + start.wait().await; - // Every concurrent resend must succeed; the commit lock must never - // starve a waiter into a timeout. - let mut results = Vec::new(); - while let Some(joined) = tasks.join_next().await { - let outcome = joined.expect("put_object_part task should not panic"); - results.push(outcome.expect("every concurrent same-part resend must succeed without lock timeout")); - } - results - }) - .await; + // The first writer holds the uploadId commit lock while the other + // resends reach the same critical section. Releasing it proves the + // handoff without depending on saturated CI disk latency. + commit_barrier.wait_until_paused().await; + commit_barrier.release(); + + let mut results = Vec::new(); + while let Some(joined) = tasks.join_next().await { + let outcome = joined.expect("put_object_part task should not panic"); + results.push(outcome.expect("every concurrent same-part resend must succeed without lock timeout")); + } assert_eq!(results.len(), candidates.len()); // Exactly one generation is visible after the serialized commits, and its diff --git a/crates/ecstore/src/set_disk/mod.rs b/crates/ecstore/src/set_disk/mod.rs index bdd66c945..692571243 100644 --- a/crates/ecstore/src/set_disk/mod.rs +++ b/crates/ecstore/src/set_disk/mod.rs @@ -687,6 +687,8 @@ mod core; mod ctx; mod metadata; mod ops; +#[cfg(test)] +pub(crate) use ops::multipart::{MultipartCommitBarrier, MultipartCommitPause}; #[cfg(feature = "test-util")] pub(crate) use ops::object::TransitionCleanupStoreBarrier as SetDiskTransitionCleanupStoreBarrier; pub(crate) use ops::object::body_cache_plaintext_len; diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index fd3416fa8..790e3a519 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -26,6 +26,8 @@ use crate::crash_inject::{self, CrashPoint}; use crate::multipart_listing::paginate_multipart_listing; use futures::{StreamExt, stream}; use std::future::Future; +#[cfg(test)] +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use tokio::task::JoinSet; @@ -33,7 +35,7 @@ const MULTIPART_LIST_IO_CONCURRENCY: usize = 16; #[cfg(test)] #[derive(Clone, Copy, PartialEq, Eq)] -enum MultipartCommitPause { +pub(crate) enum MultipartCommitPause { PutPartBeforeLockLost, PutPartAfterRename, BeforeLockLost, @@ -45,12 +47,13 @@ struct MultipartCommitBarrierState { bucket: String, object: String, pause: MultipartCommitPause, + armed: AtomicBool, arrived: tokio::sync::Notify, release: tokio::sync::Notify, } #[cfg(test)] -struct MultipartCommitBarrier { +pub(crate) struct MultipartCommitBarrier { state: Arc, } @@ -60,11 +63,12 @@ static MULTIPART_COMMIT_BARRIER: std::sync::OnceLock Self { + pub(crate) fn install(bucket: &str, object: &str, pause: MultipartCommitPause) -> Self { let state = Arc::new(MultipartCommitBarrierState { bucket: bucket.to_string(), object: object.to_string(), pause, + armed: AtomicBool::new(true), arrived: tokio::sync::Notify::new(), release: tokio::sync::Notify::new(), }); @@ -78,13 +82,13 @@ impl MultipartCommitBarrier { Self { state } } - async fn wait_until_paused(&self) { + pub(crate) async fn wait_until_paused(&self) { tokio::time::timeout(Duration::from_secs(30), self.state.arrived.notified()) .await .expect("multipart completion should reach the deterministic commit barrier"); } - fn release(&self) { + pub(crate) fn release(&self) { self.state.release.notify_one(); } } @@ -112,7 +116,9 @@ async fn pause_multipart_commit(bucket: &str, object: &str, pause: MultipartComm .as_ref() .filter(|barrier| barrier.bucket == bucket && barrier.object == object && barrier.pause == pause) .cloned(); - if let Some(barrier) = barrier { + if let Some(barrier) = barrier + && barrier.armed.swap(false, Ordering::AcqRel) + { barrier.arrived.notify_one(); barrier.release.notified().await; } diff --git a/crates/ecstore/src/store/object.rs b/crates/ecstore/src/store/object.rs index 2a8d30ee4..159fbafa3 100644 --- a/crates/ecstore/src/store/object.rs +++ b/crates/ecstore/src/store/object.rs @@ -2601,10 +2601,10 @@ mod tests { // (backlog#1304): restore entry no longer serializes on the object lock. // The replacement semantics — non-blocking reads during the copy-back and // fast rejection of a concurrent restore — are covered end-to-end by - // `restore_object_usecase_reports_ongoing_conflict_and_completion` - // (rustfs/src/app/lifecycle_transition_api_test.rs) and at the lock level - // by the accept-guard test below; restore-vs-reader data protection lives - // in the inner put_object/complete_multipart_upload commit locks. + // `restore_object_usecase_reports_ongoing_conflict` + // (rustfs/src/app/lifecycle_transition_api_test.rs), while the SetDisks + // transition matrix covers the final local commit. Restore-vs-reader data + // protection lives in the inner put_object/complete_multipart_upload locks. #[tokio::test] #[serial_test::serial] async fn restore_accept_guard_serializes_concurrent_accepts() { diff --git a/rustfs/src/app/lifecycle_transition_api_test.rs b/rustfs/src/app/lifecycle_transition_api_test.rs index 77aa6dcfd..2053ebe5c 100644 --- a/rustfs/src/app/lifecycle_transition_api_test.rs +++ b/rustfs/src/app/lifecycle_transition_api_test.rs @@ -60,7 +60,6 @@ use uuid::Uuid; static GLOBAL_ENV: OnceLock<(Vec, Arc)> = OnceLock::new(); static INIT: Once = Once::new(); const TRANSITION_WAIT_TIMEOUT: Duration = Duration::from_secs(15); -const RESTORE_COPY_BACK_WAIT_TIMEOUT: Duration = Duration::from_secs(60); const ENV_GET_CODEC_STREAMING_ENABLE: &str = "RUSTFS_GET_CODEC_STREAMING_ENABLE"; const ENV_GET_CODEC_STREAMING_ROLLOUT: &str = "RUSTFS_GET_CODEC_STREAMING_ROLLOUT"; const ENV_GET_CODEC_STREAMING_BODY_COMPAT_CONFIRMED: &str = "RUSTFS_GET_CODEC_STREAMING_BODY_COMPAT_CONFIRMED"; @@ -339,45 +338,6 @@ async fn wait_for_transition(ecstore: &Arc, bucket: &str, object: &str, } } -async fn wait_for_restore_completion( - ecstore: &Arc, - backend: &MockWarmBackend, - bucket: &str, - object: &str, - timeout: Duration, -) -> Result { - let deadline = tokio::time::Instant::now() + timeout; - let mut last_state = None; - - loop { - if tokio::time::Instant::now() >= deadline { - let tier_gets = backend.get_count().await; - let op_log = backend.op_log().await; - return Err(format!( - "restore copy-back should complete within {timeout:?}; tier_gets={tier_gets}, op_log={op_log:?}; last observed state: {}", - last_state.unwrap_or_else(|| "no object info observed".to_string()) - )); - } - - match (**ecstore).get_object_info(bucket, object, &ObjectOptions::default()).await { - Ok(info) => { - if !info.restore_ongoing && info.restore_expires.is_some() { - return Ok(info); - } - last_state = Some(format!( - "restore_ongoing={}, restore_expires={:?}, transitioned_status={}", - info.restore_ongoing, info.restore_expires, info.transitioned_object.status - )); - } - Err(err) => { - last_state = Some(format!("get_object_info failed: {err}")); - } - } - - tokio::time::sleep(Duration::from_millis(500)).await; - } -} - // SAFETY: this helper is used only by `#[serial]` tests and runs under the single-threaded Tokio // runtime (`worker_threads = 1`), so no concurrent test can mutate process environment during the // `env::set_var` / `env::remove_var` window. @@ -2097,10 +2057,9 @@ async fn put_bucket_lifecycle_configuration_rejects_zero_day_expiration() { /// POST restore(days=1) is accepted and flips the object to /// `x-amz-restore: ongoing-request="true"` while the mock tier GET barrier /// proves the background copy-back has reached the remote read; a second POST -/// during that window is rejected with 409 `RestoreAlreadyInProgress`; once the -/// copy-back completes the object reports `ongoing-request="false"` with a -/// future expiry-date; and a full GET is then served from the local restored -/// copy (the mock tier records no further `get` calls). +/// during that window is rejected with 409 `RestoreAlreadyInProgress`. +/// Synchronous SetDisks transition tests cover copy-back completion, restore +/// metadata, and local byte-identical reads. /// /// Re-enabled in the serial lane by backlog#1304: the accept path now flips /// the ongoing flag under a short compare-and-set guard and the copy-back @@ -2110,7 +2069,7 @@ async fn put_bucket_lifecycle_configuration_rejects_zero_day_expiration() { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[serial] #[ignore = "global-state ILM integration test: runs serialized in the CI ILM Integration (serial) lane, see ci.yml test-ilm-integration-serial and rustfs/backlog#1148 (ilm-8)"] -async fn restore_object_usecase_reports_ongoing_conflict_and_completion() { +async fn restore_object_usecase_reports_ongoing_conflict() { let (_disk_paths, ecstore) = setup_test_env().await; let usecase = DefaultObjectUsecase::from_global(); @@ -2177,35 +2136,6 @@ async fn restore_object_usecase_reports_ongoing_conflict_and_completion() { ); get_barrier.release(); - - // Completion: ongoing flips to false and a future expiry-date appears. - let completed = - wait_for_restore_completion(&ecstore, &backend, bucket.as_str(), object, RESTORE_COPY_BACK_WAIT_TIMEOUT).await; - let completed = completed.unwrap_or_else(|err| panic!("{err}")); - - let now_secs = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("clock before unix epoch") - .as_secs() as i64; - let expires = completed.restore_expires.expect("completed restore carries an expiry"); - assert!( - expires.unix_timestamp() > now_secs, - "restore expiry-date must be in the future, got {expires}" - ); - assert_eq!( - completed.transitioned_object.status, "complete", - "restore must not clear the transitioned state" - ); - - // The restored copy serves GET locally: no further tier GETs. - let tier_gets_after_restore = backend.get_count().await; - let data = read_object_bytes(&ecstore, bucket.as_str(), object).await; - assert_eq!(data, payload, "restored GET must return the original bytes"); - assert_eq!( - backend.get_count().await, - tier_gets_after_restore, - "GET of a restored object must be served locally, not from the tier" - ); } /// backlog#1304: the restore-accept compare-and-set itself, under real From c1538cf1c3227761c5bbd749694eb00cac516c59 Mon Sep 17 00:00:00 2001 From: cxymds Date: Wed, 29 Jul 2026 09:49:19 +0800 Subject: [PATCH 3/8] fix(multipart): serialize complete and abort (#5356) * fix(multipart): serialize complete and abort * test(multipart): order abort-first finalization * fix(multipart): enforce quorum staging cleanup * fix(multipart): remove stale mutable binding --- crates/ecstore/src/set_disk/ops/list.rs | 23 ++ crates/ecstore/src/set_disk/ops/multipart.rs | 309 ++++++++++++++++++- 2 files changed, 323 insertions(+), 9 deletions(-) diff --git a/crates/ecstore/src/set_disk/ops/list.rs b/crates/ecstore/src/set_disk/ops/list.rs index 11384914c..85eaaa188 100644 --- a/crates/ecstore/src/set_disk/ops/list.rs +++ b/crates/ecstore/src/set_disk/ops/list.rs @@ -29,6 +29,12 @@ impl SetDisks { pub async fn delete_all(&self, bucket: &str, prefix: &str) -> Result<()> { ListOperations::new(self.ctx()).delete_all(bucket, prefix).await } + + pub(crate) async fn delete_all_with_quorum(&self, bucket: &str, prefix: &str, write_quorum: usize) -> Result<()> { + ListOperations::new(self.ctx()) + .delete_all_with_quorum(bucket, prefix, write_quorum) + .await + } } /// List/prefix maintenance operations, borrowing the `SetDisks` core state @@ -48,6 +54,14 @@ impl<'a> ListOperations<'a> { } pub(crate) async fn delete_all(&self, bucket: &str, prefix: &str) -> Result<()> { + self.delete_all_inner(bucket, prefix, None).await + } + + async fn delete_all_with_quorum(&self, bucket: &str, prefix: &str, write_quorum: usize) -> Result<()> { + self.delete_all_inner(bucket, prefix, Some(write_quorum)).await + } + + async fn delete_all_inner(&self, bucket: &str, prefix: &str, write_quorum: Option) -> Result<()> { let disks = self.ctx.disks().read().await; let disks = disks.clone(); @@ -79,6 +93,9 @@ impl<'a> ListOperations<'a> { Ok(_) => { errors.push(None); } + Err(DiskError::FileNotFound | DiskError::PathNotFound | DiskError::VolumeNotFound) => { + errors.push(None); + } Err(e) => { errors.push(Some(e)); } @@ -97,6 +114,12 @@ impl<'a> ListOperations<'a> { ); } + if let Some(write_quorum) = write_quorum + && let Some(err) = reduce_write_quorum_errs(&errors, OBJECT_OP_IGNORED_ERRS, write_quorum) + { + return Err(err.into()); + } + Ok(()) } } diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index 790e3a519..6eaaa3399 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -783,6 +783,9 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { mut max_parts: usize, opts: &ObjectOptions, ) -> Result { + let _upload_guard = self + .acquire_multipart_upload_read_lock("list_object_parts", bucket, object, upload_id, opts) + .await?; let (fi, _) = self.check_upload_id_exists(bucket, object, upload_id, false).await?; let upload_id_path = Self::get_upload_id_dir(bucket, object, upload_id); @@ -1260,10 +1263,15 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { let _upload_guard = self .acquire_multipart_upload_write_lock("abort_multipart_upload", bucket, object, upload_id, opts) .await?; - self.check_upload_id_exists(bucket, object, upload_id, false).await?; + let (fi, _) = self.check_upload_id_exists(bucket, object, upload_id, true).await?; let upload_id_path = Self::get_upload_id_dir(bucket, object, upload_id); - self.delete_all(RUSTFS_META_MULTIPART_BUCKET, &upload_id_path).await + self.delete_all_with_quorum( + RUSTFS_META_MULTIPART_BUCKET, + &upload_id_path, + fi.write_quorum(self.default_write_quorum()), + ) + .await } // complete_multipart_upload finished #[tracing::instrument(skip(self))] @@ -1821,7 +1829,36 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { #[cfg(test)] pause_multipart_commit(bucket, object, MultipartCommitPause::AfterRename).await; - drop(upload_guard); + + let cleanup_store = self.clone(); + let cleanup_upload_id_path = upload_id_path.clone(); + let cleanup_bucket = bucket.to_owned(); + let cleanup_object = object.to_owned(); + let cleanup_upload_id = upload_id.to_owned(); + let cleanup_handle = tokio::spawn(async move { + let _upload_guard = upload_guard; + if let Err(err) = cleanup_store + .delete_all_with_quorum(RUSTFS_META_MULTIPART_BUCKET, &cleanup_upload_id_path, write_quorum) + .await + { + warn!( + bucket = %cleanup_bucket, + object = %cleanup_object, + upload_id = %cleanup_upload_id, + error = ?err, + "completed multipart upload staging cleanup did not reach write quorum" + ); + } + }); + if let Err(err) = cleanup_handle.await { + warn!( + bucket = %bucket, + object = %object, + upload_id = %upload_id, + error = ?err, + "completed multipart upload staging cleanup task failed" + ); + } drop(object_lock_guard); // drop object lock guard to release the lock // backlog#1321: enqueue heal only when the committed replicas actually @@ -1866,12 +1903,6 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { }); } - let upload_id_path = upload_id_path.clone(); - let store = self.clone(); - let _cleanup_handle = tokio::spawn(async move { - let _ = store.delete_all(RUSTFS_META_MULTIPART_BUCKET, &upload_id_path).await; - }); - for (i, op_disk) in online_disks.iter().enumerate() { if let Some(disk) = op_disk && disk.is_online().await @@ -2183,6 +2214,122 @@ mod tests { ) } + async fn assert_complete_first_linearizes(bucket: &'static str, object: &'static str, create_opts: ObjectOptions) { + let manager = Arc::new(rustfs_lock::GlobalLockManager::new()); + let signaling = Arc::new(SignalingLockClient::new(Arc::new(LocalClient::with_manager(manager)))); + let lockers: Vec> = vec![signaling.clone()]; + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_with_lockers(4, 0, 2, lockers).await; + make_bucket_on_all(&disk_stores, bucket).await; + let (upload_id, parts) = stage_upload_with_create_opts(&set_disks, bucket, object, &[0x47; 4096], &create_opts).await; + let upload_id_path = SetDisks::get_upload_id_dir(bucket, object, &upload_id); + signaling.set_target(rustfs_lock::ObjectKey::new(RUSTFS_META_MULTIPART_BUCKET, upload_id_path)); + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + let barrier = MultipartCommitBarrier::install(bucket, object, MultipartCommitPause::AfterRename); + + let complete_store = set_disks.clone(); + let complete_upload_id = upload_id.clone(); + let complete = tokio::spawn(async move { + complete_store + .complete_multipart_upload(bucket, object, &complete_upload_id, parts, &ObjectOptions::default()) + .await + }); + barrier.wait_until_paused().await; + + let abort_store = set_disks.clone(); + let abort_upload_id = upload_id.clone(); + let abort = tokio::spawn(async move { + abort_store + .abort_multipart_upload(bucket, object, &abort_upload_id, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(2).await; + assert!(!abort.is_finished(), "abort must wait for the completion upload lock"); + + barrier.release(); + complete + .await + .expect("completion task should not panic") + .expect("completion should win the upload finalization"); + let abort_err = abort + .await + .expect("abort task should not panic") + .expect_err("abort must observe the upload as finalized"); + assert!(matches!(abort_err, StorageError::InvalidUploadID(..))); + set_disks + .get_object_info(bucket, object, &ObjectOptions::default()) + .await + .expect("complete-first must leave the committed object readable"); + assert!(matches!( + set_disks.check_upload_id_exists(bucket, object, &upload_id, false).await, + Err(StorageError::InvalidUploadID(..)) + )); + } + + async fn assert_abort_first_linearizes(bucket: &'static str, object: &'static str, create_opts: ObjectOptions) { + let manager = Arc::new(rustfs_lock::GlobalLockManager::new()); + let signaling = Arc::new(SignalingLockClient::new(Arc::new(LocalClient::with_manager(manager)))); + let lockers: Vec> = vec![signaling.clone()]; + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_with_lockers(4, 0, 2, lockers).await; + make_bucket_on_all(&disk_stores, bucket).await; + let (upload_id, parts) = stage_upload_with_create_opts(&set_disks, bucket, object, &[0x48; 4096], &create_opts).await; + let upload_id_path = SetDisks::get_upload_id_dir(bucket, object, &upload_id); + signaling.set_target(rustfs_lock::ObjectKey::new(RUSTFS_META_MULTIPART_BUCKET, upload_id_path.clone())); + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + let object_holder = set_disks + .new_ns_lock(bucket, object) + .await + .expect("object namespace lock should be created") + .get_write_lock(Duration::from_secs(5)) + .await + .expect("test should hold the object lock"); + let holder = set_disks + .new_ns_lock(RUSTFS_META_MULTIPART_BUCKET, &upload_id_path) + .await + .expect("upload namespace lock should be created") + .get_write_lock(Duration::from_secs(5)) + .await + .expect("test should hold the upload lock"); + signaling.wait_for_attempts(1).await; + + let abort_store = set_disks.clone(); + let abort_upload_id = upload_id.clone(); + let abort = tokio::spawn(async move { + abort_store + .abort_multipart_upload(bucket, object, &abort_upload_id, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(2).await; + + let complete_store = set_disks.clone(); + let complete_upload_id = upload_id.clone(); + let complete = tokio::spawn(async move { + complete_store + .complete_multipart_upload(bucket, object, &complete_upload_id, parts, &ObjectOptions::default()) + .await + }); + drop(holder); + + abort + .await + .expect("abort task should not panic") + .expect("abort should win the upload finalization"); + drop(object_holder); + let complete_err = complete + .await + .expect("completion task should not panic") + .expect_err("completion must observe the aborted upload"); + assert!(matches!(complete_err, StorageError::InvalidUploadID(..))); + let object_err = set_disks + .get_object_info(bucket, object, &ObjectOptions::default()) + .await + .expect_err("abort-first must not publish an object"); + assert!(matches!(object_err, StorageError::ObjectNotFound(..))); + assert!(matches!( + set_disks.check_upload_id_exists(bucket, object, &upload_id, false).await, + Err(StorageError::InvalidUploadID(..)) + )); + } + async fn assert_quorum_minus_one_retry_preserves_completable_part( disk_count: usize, parity: usize, @@ -3045,6 +3192,81 @@ mod tests { .await; } + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn abort_and_complete_linearize_for_plain_sse_and_legacy_layouts() { + assert_complete_first_linearizes("multipart-complete-first-plain", "object", ObjectOptions::default()).await; + assert_abort_first_linearizes("multipart-abort-first-plain", "object", ObjectOptions::default()).await; + + let encrypted_opts = ObjectOptions { + user_defined: HashMap::from([(SSEC_ALGORITHM_HEADER.to_string(), "AES256".to_string())]), + ..Default::default() + }; + temp_env::async_with_vars([(crate::object_api::ENV_RUSTFS_ENCRYPTED_RANGE_SEEK, Some("true"))], async { + assert_complete_first_linearizes("multipart-complete-first-sse", "object", encrypted_opts.clone()).await; + assert_abort_first_linearizes("multipart-abort-first-sse", "object", encrypted_opts.clone()).await; + }) + .await; + temp_env::async_with_vars([(crate::object_api::ENV_RUSTFS_ENCRYPTED_RANGE_SEEK, Some("false"))], async { + assert_complete_first_linearizes("multipart-complete-first-legacy", "object", encrypted_opts.clone()).await; + assert_abort_first_linearizes("multipart-abort-first-legacy", "object", encrypted_opts).await; + }) + .await; + } + + #[tokio::test] + async fn abort_enforces_delete_write_quorum_boundary() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "multipart-abort-delete-quorum"; + let object = "object"; + make_bucket_on_all(&disk_stores, bucket).await; + let quorum_upload = set_disks + .new_multipart_upload(bucket, object, &ObjectOptions::default()) + .await + .expect("multipart upload should be created"); + + let saved_disks = { + let mut disks = set_disks.disks.write().await; + let saved = disks.clone(); + disks[3] = None; + saved + }; + set_disks + .abort_multipart_upload(bucket, object, &quorum_upload.upload_id, &ObjectOptions::default()) + .await + .expect("abort should succeed at the exact delete write quorum"); + *set_disks.disks.write().await = saved_disks; + assert!(matches!( + set_disks + .check_upload_id_exists(bucket, object, &quorum_upload.upload_id, false) + .await, + Err(StorageError::InvalidUploadID(..)) + )); + + let below_quorum_upload = set_disks + .new_multipart_upload(bucket, object, &ObjectOptions::default()) + .await + .expect("second multipart upload should be created"); + let saved_disks = { + let mut disks = set_disks.disks.write().await; + let saved = disks.clone(); + disks[2] = None; + disks[3] = None; + saved + }; + let err = set_disks + .abort_multipart_upload(bucket, object, &below_quorum_upload.upload_id, &ObjectOptions::default()) + .await + .expect_err("abort must report a delete below write quorum"); + assert!(matches!(err, StorageError::ErasureWriteQuorum)); + + *set_disks.disks.write().await = saved_disks; + set_disks + .check_upload_id_exists(bucket, object, &below_quorum_upload.upload_id, false) + .await + .expect("failed abort must leave quorum-visible staging on the restored disks"); + } + #[tokio::test(flavor = "multi_thread")] #[serial] async fn complete_revalidates_layout_candidate_after_upload_lock() { @@ -3176,6 +3398,17 @@ mod tests { tokio::task::yield_now().await; assert!(!abort.is_finished(), "abort must wait until completion releases the upload lock"); + let list_store = set_disks.clone(); + let list_upload_id = upload_id.clone(); + let list = tokio::spawn(async move { + list_store + .list_object_parts(bucket, object, &list_upload_id, None, MAX_PARTS_COUNT, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(3).await; + tokio::task::yield_now().await; + assert!(!list.is_finished(), "ListParts must wait until completion releases the upload lock"); + barrier.release(); complete .await @@ -3186,10 +3419,68 @@ mod tests { .expect("abort task should not panic") .expect_err("the committed upload should no longer exist when abort acquires the lock"); assert!(matches!(abort_err, StorageError::InvalidUploadID(..))); + let list_err = list + .await + .expect("ListParts task should not panic") + .expect_err("the committed upload should no longer exist when ListParts acquires the lock"); + assert!(matches!(list_err, StorageError::InvalidUploadID(..))); }) .await; } + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn complete_validates_parts_after_an_inflight_upload_part_commit() { + let manager = Arc::new(rustfs_lock::GlobalLockManager::new()); + let signaling = Arc::new(SignalingLockClient::new(Arc::new(LocalClient::with_manager(manager)))); + let lockers: Vec> = vec![signaling.clone()]; + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_with_lockers(4, 0, 2, lockers).await; + let bucket = "multipart-complete-put-part-race-bucket"; + let object = "object"; + make_bucket_on_all(&disk_stores, bucket).await; + let (upload_id, original_parts) = + stage_upload_with_create_opts(&set_disks, bucket, object, &[0x49; 4096], &ObjectOptions::default()).await; + let upload_id_path = SetDisks::get_upload_id_dir(bucket, object, &upload_id); + signaling.set_target(rustfs_lock::ObjectKey::new(RUSTFS_META_MULTIPART_BUCKET, upload_id_path)); + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + let barrier = MultipartCommitBarrier::install(bucket, object, MultipartCommitPause::PutPartBeforeLockLost); + + let put_store = set_disks.clone(); + let put_upload_id = upload_id.clone(); + let put = tokio::spawn(async move { + let mut reader = PutObjReader::from_vec(vec![0x4a; 4096]); + put_store + .put_object_part(bucket, object, &put_upload_id, 1, &mut reader, &ObjectOptions::default()) + .await + }); + barrier.wait_until_paused().await; + + let complete_store = set_disks.clone(); + let complete_upload_id = upload_id.clone(); + let complete = tokio::spawn(async move { + complete_store + .complete_multipart_upload(bucket, object, &complete_upload_id, original_parts, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(2).await; + tokio::task::yield_now().await; + assert!(!complete.is_finished(), "completion must wait for the UploadPart commit lock"); + + barrier.release(); + put.await + .expect("UploadPart task should not panic") + .expect("UploadPart replacement should commit"); + let err = complete + .await + .expect("completion task should not panic") + .expect_err("completion must reject the stale ETag after UploadPart wins"); + assert!(matches!(err, StorageError::InvalidPart(..))); + set_disks + .list_object_parts(bucket, object, &upload_id, None, MAX_PARTS_COUNT, &ObjectOptions::default()) + .await + .expect("failed completion must leave the upload retryable"); + } + #[tokio::test(start_paused = true)] #[serial] async fn complete_fences_upload_lock_loss_before_commit() { From 957080bea5cde950ff16a777a69d2020c8e68451 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Wed, 29 Jul 2026 10:31:11 +0800 Subject: [PATCH 4/8] fix(swift): persist container and account metadata writes (#5398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swift container and account metadata handlers cloned the cached BucketMetadata, set the tagging fields, and called set_bucket_metadata, which only updates the in-memory cache map. Nothing reached .metadata.bin, so every Swift metadata POST was lost on restart and silently overwritten by the next disk-truth reload (a peer LoadBucketMetadata notification or the 15-minute refresh loop) — while the client had already been told 2xx. Route these writes through a new metadata_sys::update_config_with: a read-modify-write that loads the on-disk metadata and persists the result under the same write guard metadata_sys::update uses, so the rewrite merges against disk truth instead of a possibly stale cache and cannot clobber a concurrent update to another config file. Peers are notified afterwards, matching the S3 config handlers. Persisting these writes required hardening the paths that now produce durable state: - Account metadata writes validate account ownership. This metadata holds the account's TempURL signing key, so an unauthenticated write for someone else's account would have become a durable, cluster-wide takeover of that account's pre-signed URLs. Reads stay open because TempURL signature validation runs before credentials exist. - disable_versioning verifies the container exists. Without it the metadata loader's "no metadata on disk" default would be persisted, creating an orphan metadata file and caching a fabricated default as authoritative. - Container and account metadata are size- and count-limited, reusing the Swift limits object metadata already enforces; these tags land in the bucket metadata file that every later config write rewrites whole. - A rewrite refuses to run when the persisted tagging config is unreadable, instead of merging onto an empty set and wiping the container ACL and versioning tags. It reports 409 naming the remedy. - Storage errors are logged in full and reported generically, since they now carry real disk and quorum detail. The tagging arm of BucketMetadata::update_config also clears the parsed config, as the lifecycle arm does: parse_all_configs skips empty XML rather than clearing, so a cleared config kept serving the old tags. Tagging is serialized with the S3 XML serializer the loader can parse back, not quick_xml, whose output was never round-trippable. --- Cargo.lock | 1 + crates/ecstore/src/api/mod.rs | 2 +- crates/ecstore/src/bucket/metadata.rs | 27 ++ crates/ecstore/src/bucket/metadata_sys.rs | 207 +++++++++++- crates/protocols/Cargo.toml | 1 + crates/protocols/src/swift/account.rs | 92 +++--- crates/protocols/src/swift/container.rs | 270 ++++++--------- crates/protocols/src/swift/mod.rs | 45 ++- crates/protocols/src/swift/object.rs | 40 +-- crates/protocols/src/swift/storage_api.rs | 98 +++++- .../tests/ecstore_test_compat/mod.rs | 25 ++ .../tests/swift_metadata_persistence.rs | 312 ++++++++++++++++++ 12 files changed, 829 insertions(+), 291 deletions(-) create mode 100644 crates/protocols/tests/ecstore_test_compat/mod.rs create mode 100644 crates/protocols/tests/swift_metadata_persistence.rs diff --git a/Cargo.lock b/Cargo.lock index 1dfac4a90..e824ff26a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9732,6 +9732,7 @@ dependencies = [ "rustfs-policy", "rustfs-rio", "rustfs-storage-api", + "rustfs-test-utils", "rustfs-tls-runtime", "rustfs-trusted-proxies", "rustfs-utils", diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index fc042f97f..3491103ef 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -128,7 +128,7 @@ pub mod bucket { get_object_lock_config, get_public_access_block_config, get_quota_config, get_replication_config, get_request_payment_config, get_sse_config, get_tagging_config, get_versioning_config, get_website_config, init_bucket_metadata_sys, list_bucket_targets, remove_bucket_metadata, set_bucket_metadata, update, - update_bucket_targets_under_transaction_lock, + update_bucket_targets_under_transaction_lock, update_config_with, }; } diff --git a/crates/ecstore/src/bucket/metadata.rs b/crates/ecstore/src/bucket/metadata.rs index eddce55fc..441036227 100644 --- a/crates/ecstore/src/bucket/metadata.rs +++ b/crates/ecstore/src/bucket/metadata.rs @@ -737,6 +737,9 @@ impl BucketMetadata { } BUCKET_TAGGING_CONFIG => { self.tagging_config_xml = data; + // Drop the parsed form (like lifecycle above) so clearing the + // payload can't leave stale parsed tags to be cached. + self.tagging_config = None; self.tagging_config_updated_at = updated; } BUCKET_QUOTA_CONFIG_FILE => { @@ -1318,6 +1321,30 @@ mod test { assert!(bm.lifecycle_config.is_none()); } + /// Companion to the lifecycle case above. `parse_all_configs` skips empty + /// XML rather than clearing, so without the explicit reset a cleared + /// tagging config would keep serving the previously parsed tags. + #[test] + fn tagging_update_config_clears_parsed_config_on_delete() { + let mut bm = BucketMetadata::new("test-bucket"); + let tagging_xml = br#"envprod"#; + + bm.update_config(BUCKET_TAGGING_CONFIG, tagging_xml.to_vec()) + .expect("tagging config should update"); + bm.parse_all_configs().expect("tagging config should parse"); + assert!(bm.tagging_config.is_some()); + + bm.update_config(BUCKET_TAGGING_CONFIG, Vec::new()) + .expect("tagging config delete should update metadata"); + + assert!(bm.tagging_config_xml.is_empty()); + assert!(bm.tagging_config.is_none()); + + // A re-parse must not resurrect them either. + bm.parse_all_configs().expect("cleared tagging should parse"); + assert!(bm.tagging_config.is_none()); + } + #[tokio::test] async fn marshal_msg_complete_example() { // Create a complete BucketMetadata with various configurations diff --git a/crates/ecstore/src/bucket/metadata_sys.rs b/crates/ecstore/src/bucket/metadata_sys.rs index 97dbf9147..dbd9bb573 100644 --- a/crates/ecstore/src/bucket/metadata_sys.rs +++ b/crates/ecstore/src/bucket/metadata_sys.rs @@ -239,6 +239,35 @@ pub async fn update_bucket_targets_under_transaction_lock(bucket: &str, data: Ve bucket_meta_sys.update(bucket, BUCKET_TARGETS_FILE, data).await } +/// Read-modify-write one bucket config file under the metadata system's +/// outer write guard. +/// +/// `mutate` sees the freshly loaded on-disk metadata and returns the +/// replacement payload for `config_file` (empty clears it, like +/// [`delete`]). Both the read and the persisted write happen inside the +/// same guard that [`update`] uses, so within this process the rewrite can +/// neither clobber a concurrent update to another config file nor lose a +/// concurrent write to the same one — unlike caching a mutated clone of +/// previously read metadata. +/// +/// This guard is process-local. Writers on other nodes still race, exactly +/// as they do for [`update`]: each rewrites the whole metadata file, so the +/// later save wins. What this narrows is the window — from "as stale as the +/// local cache" down to a single metadata read plus write. +pub async fn update_config_with(bucket: &str, config_file: &str, mutate: F) -> Result +where + F: FnOnce(&BucketMetadata) -> Result> + Send, +{ + let bucket_meta_sys_lock = get_bucket_metadata_sys()?; + let _targets_guard = if config_file == BUCKET_TARGETS_FILE { + Some(acquire_bucket_targets_transaction_lock(bucket).await?) + } else { + None + }; + let mut bucket_meta_sys = bucket_meta_sys_lock.write().await; + bucket_meta_sys.update_config_with(bucket, config_file, mutate).await +} + pub async fn acquire_bucket_targets_transaction_lock(bucket: &str) -> Result { let bucket_meta_sys_lock = get_bucket_metadata_sys()?; let api = bucket_meta_sys_lock.read().await.object_store(); @@ -603,24 +632,7 @@ impl BucketMetadataSys { return Err(Error::other("errServerNotInitialized")); }; - if is_meta_bucketname(bucket) { - return Err(Error::other("errInvalidArgument")); - } - - let mut bm = match load_bucket_metadata_parse(store, bucket, parse).await { - Ok(res) => res, - Err(err) => { - if !runtime_sources::setup_is_erasure().await - && !runtime_sources::setup_is_dist_erasure().await - && is_err_bucket_not_found(&err) - { - BucketMetadata::new(bucket) - } else { - error!("load bucket metadata failed: {}", err); - return Err(err); - } - } - }; + let mut bm = Self::load_bucket_metadata_for_update(store, bucket, parse).await?; let updated = bm.update_config(config_file, data)?; @@ -629,6 +641,49 @@ impl BucketMetadataSys { Ok(updated) } + /// See the free [`update_config_with`]: same load-mutate-persist cycle as + /// [`Self::update`], with the payload computed from the loaded metadata + /// instead of supplied up front. Loads through this system's own store so + /// the read and the persisted write target the same instance. + async fn update_config_with(&mut self, bucket: &str, config_file: &str, mutate: F) -> Result + where + F: FnOnce(&BucketMetadata) -> Result> + Send, + { + let mut bm = Self::load_bucket_metadata_for_update(self.api.clone(), bucket, true).await?; + + let data = mutate(&bm)?; + let updated = bm.update_config(config_file, data)?; + + self.save(bm).await?; + + Ok(updated) + } + + /// Load a bucket's on-disk metadata as the base of a config rewrite. + /// Outside erasure setups a missing metadata file degrades to a fresh + /// default (legacy buckets without one); erasure setups fail instead of + /// fabricating state that a quorum may still hold. + async fn load_bucket_metadata_for_update(store: Arc, bucket: &str, parse: bool) -> Result { + if is_meta_bucketname(bucket) { + return Err(Error::other("errInvalidArgument")); + } + + match load_bucket_metadata_parse(store, bucket, parse).await { + Ok(res) => Ok(res), + Err(err) => { + if !runtime_sources::setup_is_erasure().await + && !runtime_sources::setup_is_dist_erasure().await + && is_err_bucket_not_found(&err) + { + Ok(BucketMetadata::new(bucket)) + } else { + error!("load bucket metadata failed: {}", err); + Err(err) + } + } + } + } + async fn save(&self, bm: BucketMetadata) -> Result<()> { if is_meta_bucketname(&bm.name) { return Err(Error::other("errInvalidArgument")); @@ -1068,6 +1123,122 @@ mod tests { assert!(matches!(err, Error::Io(_)), "malformed persisted policy must surface its parse failure"); } + /// A tagging rewrite through `update_config_with` (the Swift metadata + /// POST path) is persisted: it survives a metadata reload from disk, and + /// an emptied rewrite clears the config in the cached copy too instead of + /// leaving stale parsed tags behind. + #[tokio::test] + async fn update_config_with_persists_tagging_rewrite_across_disk_reload() { + use crate::bucket::metadata::BUCKET_TAGGING_CONFIG; + use s3s::dto::Tag; + + let (_dirs, ecstore) = isolated_store_over_temp_disks().await; + let mut sys = BucketMetadataSys::new(ecstore); + + let bucket = "swift-tagging-bucket"; + sys.persist_and_set(BucketMetadata::new(bucket)) + .await + .expect("initial metadata should persist"); + + let tagging = Tagging { + tag_set: vec![Tag { + key: Some("swift-meta-color".to_string()), + value: Some("blue".to_string()), + }], + }; + let xml = crate::bucket::utils::serialize::(&tagging).expect("tagging should serialize"); + sys.update_config_with(bucket, BUCKET_TAGGING_CONFIG, move |bm| { + assert!(bm.tagging_config.is_none(), "rewrite must see the on-disk state"); + Ok(xml) + }) + .await + .expect("tagging rewrite should persist"); + + // Simulate the disk-truth reload that used to lose Swift writes: drop + // the cached entry and lazily re-load from the metadata file. + sys.metadata_map.write().await.clear(); + let (tags, _) = sys + .get_tagging_config(bucket) + .await + .expect("tagging must survive a reload from disk"); + assert_eq!(tags.tag_set.len(), 1); + assert_eq!(tags.tag_set[0].key.as_deref(), Some("swift-meta-color")); + assert_eq!(tags.tag_set[0].value.as_deref(), Some("blue")); + + // An emptied rewrite clears the config everywhere. + sys.update_config_with(bucket, BUCKET_TAGGING_CONFIG, |bm| { + assert!(bm.tagging_config.is_some(), "rewrite must see the persisted tags"); + Ok(Vec::new()) + }) + .await + .expect("clearing rewrite should persist"); + assert_eq!( + sys.get_tagging_config(bucket).await.unwrap_err(), + Error::ConfigNotFound, + "cleared tagging must not be served from the cache" + ); + sys.metadata_map.write().await.clear(); + assert_eq!( + sys.get_tagging_config(bucket).await.unwrap_err(), + Error::ConfigNotFound, + "cleared tagging must not reappear after a reload from disk" + ); + } + + /// The load and the persisted write share one write guard, so concurrent + /// rewrites of the same config compose instead of clobbering each other. + /// Moving the load outside that guard loses all but the last tag. + #[tokio::test] + async fn concurrent_update_config_with_calls_do_not_lose_writes() { + use crate::bucket::metadata::BUCKET_TAGGING_CONFIG; + use s3s::dto::Tag; + + let (_dirs, ecstore) = isolated_store_over_temp_disks().await; + let sys = Arc::new(RwLock::new(BucketMetadataSys::new(ecstore))); + + let bucket = "swift-tagging-concurrent"; + sys.read() + .await + .persist_and_set(BucketMetadata::new(bucket)) + .await + .expect("initial metadata should persist"); + + const WRITERS: usize = 8; + let mut handles = Vec::with_capacity(WRITERS); + for idx in 0..WRITERS { + let sys = sys.clone(); + handles.push(tokio::spawn(async move { + sys.write() + .await + .update_config_with(bucket, BUCKET_TAGGING_CONFIG, move |bm| { + // Each writer merges its own tag onto whatever is + // currently persisted — the Swift rewrite shape. + let mut tagging = bm.tagging_config.clone().unwrap_or_else(|| Tagging { tag_set: vec![] }); + tagging.tag_set.push(Tag { + key: Some(format!("swift-meta-key{idx}")), + value: Some(idx.to_string()), + }); + crate::bucket::utils::serialize::(&tagging).map_err(|e| Error::other(e.to_string())) + }) + .await + })); + } + + for handle in handles { + handle + .await + .expect("writer task should join") + .expect("rewrite should persist"); + } + + let (tags, _) = sys + .read() + .await + .get_tagging_config(bucket) + .await + .expect("tagging should be readable"); + assert_eq!(tags.tag_set.len(), WRITERS, "every concurrent rewrite must survive: {tags:?}"); + } fn target(bucket: &str, id: &str) -> BucketTarget { BucketTarget { diff --git a/crates/protocols/Cargo.toml b/crates/protocols/Cargo.toml index d9ea48b41..003e4c4af 100644 --- a/crates/protocols/Cargo.toml +++ b/crates/protocols/Cargo.toml @@ -134,6 +134,7 @@ socket2 = { workspace = true, optional = true, features = ["all"] } [dev-dependencies] tempfile = { workspace = true } proptest = "1" +rustfs-test-utils = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter", "time"] } tokio = { workspace = true, features = ["test-util", "macros", "fs"] } diff --git a/crates/protocols/src/swift/account.rs b/crates/protocols/src/swift/account.rs index e76d4c850..cf3aa538c 100644 --- a/crates/protocols/src/swift/account.rs +++ b/crates/protocols/src/swift/account.rs @@ -16,12 +16,11 @@ use super::storage_api::account::{BucketOperations, MakeBucketOptions}; use super::{SwiftError, SwiftResult}; -use super::{get_swift_bucket_metadata, resolve_swift_object_store_handle, set_swift_bucket_metadata}; +use super::{get_swift_bucket_metadata, resolve_swift_object_store_handle, update_swift_bucket_tagging, validate_metadata}; use rustfs_credentials::Credentials; use s3s::dto::{Tag, Tagging}; use sha2::{Digest, Sha256}; use std::collections::HashMap; -use time; /// Validate that the authenticated user has access to the requested account /// @@ -148,15 +147,33 @@ pub async fn get_account_metadata(account: &str, _credentials: &Option, - _credentials: &Option, + credentials: &Option, ) -> SwiftResult<()> { + let Some(credentials) = credentials.as_ref() else { + return Err(SwiftError::Unauthorized( + "Keystone authentication required to update account metadata".to_string(), + )); + }; + validate_account_access(account, credentials)?; + + // These tags are persisted into the bucket metadata file, which every + // later config write rewrites in full — so unbounded metadata inflates + // the cost of unrelated writes for the life of the account. + validate_metadata(metadata)?; + let bucket_name = get_account_metadata_bucket_name(account); let Some(store) = resolve_swift_object_store_handle() else { @@ -173,57 +190,30 @@ pub async fn update_account_metadata( .map_err(|e| SwiftError::InternalServerError(format!("Failed to create account metadata bucket: {}", e)))?; } - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + // Rewrite the persisted tags: replace swift-account-meta-* tags with the + // new metadata while preserving other tags. An empty result clears the + // tagging config. + update_swift_bucket_tagging(bucket_name, |current| { + let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); - - // Get existing tags, preserving non-Swift tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); - - // Remove old swift-account-meta-* tags while preserving other tags - existing_tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-account-meta-") - } else { - true - } - }); - - // Add new metadata tags - for (key, value) in metadata { - existing_tagging.tag_set.push(Tag { - key: Some(format!("swift-account-meta-{}", key)), - value: Some(value.clone()), + tagging.tag_set.retain(|tag| { + if let Some(key) = &tag.key { + !key.starts_with("swift-account-meta-") + } else { + true + } }); - } - let now = time::OffsetDateTime::now_utc(); + for (key, value) in metadata { + tagging.tag_set.push(Tag { + key: Some(format!("swift-account-meta-{}", key)), + value: Some(value.clone()), + }); + } - if existing_tagging.tag_set.is_empty() { - // No tags remain; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name.clone(), bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } diff --git a/crates/protocols/src/swift/container.rs b/crates/protocols/src/swift/container.rs index ffbc24498..fb0ad807d 100644 --- a/crates/protocols/src/swift/container.rs +++ b/crates/protocols/src/swift/container.rs @@ -22,7 +22,10 @@ use super::storage_api::container::{ }; use super::types::Container; use super::{SwiftError, SwiftResult}; -use super::{get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, set_swift_bucket_metadata}; +use super::{ + get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging, + validate_metadata, +}; use rustfs_credentials::Credentials; use s3s::dto::{Tag, Tagging}; use sha2::{Digest, Sha256}; @@ -483,6 +486,11 @@ pub async fn update_container_metadata( // Validate container name validate_container_name(container)?; + // These tags are persisted into the bucket metadata file, which every + // later config write rewrites in full — so unbounded metadata inflates + // the cost of unrelated writes for the life of the container. + validate_metadata(&metadata)?; + // Create mapper with default config (tenant prefixing enabled) let mapper = ContainerMapper::default(); @@ -506,57 +514,30 @@ pub async fn update_container_metadata( } })?; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + // Rewrite the persisted tags: replace swift-meta-* tags with the new + // metadata while preserving non-Swift tags. An empty result clears the + // tagging config. + update_swift_bucket_tagging(bucket_name, |current| { + let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); + tagging.tag_set.retain(|tag| { + if let Some(key) = &tag.key { + !key.starts_with("swift-meta-") + } else { + true // Keep tags with no key (shouldn't happen, but be safe) + } + }); - // Get existing tags, preserving non-Swift tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); - - // Remove old swift-meta-* tags while preserving other tags - existing_tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-meta-") - } else { - true // Keep tags with no key (shouldn't happen, but be safe) + if let Some(mut new_tagging) = swift_metadata_to_s3_tags(&metadata) { + tagging.tag_set.append(&mut new_tagging.tag_set); } - }); + // If metadata.is_empty() and swift_metadata_to_s3_tags returns None, + // we've already removed swift-meta-* tags above, so only non-Swift + // tags remain - // Add new Swift metadata tags if provided - if let Some(mut new_tagging) = swift_metadata_to_s3_tags(&metadata) { - // Merge: existing non-Swift tags + new Swift tags - existing_tagging.tag_set.append(&mut new_tagging.tag_set); - } - // If metadata.is_empty() and swift_metadata_to_s3_tags returns None, - // we've already removed swift-meta-* tags above, so only non-Swift tags remain - - let now = time::OffsetDateTime::now_utc(); - - if existing_tagging.tag_set.is_empty() { - // No tags remain after removing swift-meta-* tags; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize the merged tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } @@ -819,44 +800,23 @@ pub async fn enable_versioning( } })?; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + // Rewrite the persisted tags: replace any versioning tag with the new + // archive location while preserving all other tags. + update_swift_bucket_tagging(bucket_name, |current| { + let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); + tagging + .tag_set + .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - // Get existing tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); + tagging.tag_set.push(Tag { + key: Some("swift-versions-location".to_string()), + value: Some(archive_container.to_string()), // Store Swift container name, not S3 bucket name + }); - // Remove old versioning tag if present - existing_tagging - .tag_set - .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - - // Add new versioning tag - existing_tagging.tag_set.push(Tag { - key: Some("swift-versions-location".to_string()), - value: Some(archive_container.to_string()), // Store Swift container name, not S3 bucket name - }); - - let now = time::OffsetDateTime::now_utc(); - - // Serialize tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } @@ -882,50 +842,38 @@ pub async fn disable_versioning(account: &str, container: &str, credentials: &Cr let mapper = ContainerMapper::default(); let bucket_name = mapper.swift_to_s3_bucket(container, &project_id); - // Verify container exists - let Some(_store) = resolve_swift_object_store_handle() else { + let Some(store) = resolve_swift_object_store_handle() else { return Err(SwiftError::InternalServerError("Storage layer not initialized".to_string())); }; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) + // Verify container exists. Without this the rewrite below would persist a + // fabricated default for a container that does not exist: the metadata + // loader turns "no metadata on disk" into a fresh BucketMetadata, and + // writing that creates an orphan .metadata.bin and caches a fabricated + // default as authoritative. + store + .get_bucket_info(&bucket_name, &BucketOptions::default()) .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + .map_err(|e| { + if e.to_string().contains("not found") || e.to_string().contains("NoSuchBucket") { + SwiftError::NotFound(format!("Container '{}' not found", container)) + } else { + sanitize_storage_error("Container verification", e) + } + })?; - let mut bucket_meta_clone = (*bucket_meta).clone(); + // Rewrite the persisted tags: drop the versioning tag while preserving + // all other tags. An empty result clears the tagging config. + update_swift_bucket_tagging(bucket_name, |current| { + let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); - // Get existing tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); + tagging + .tag_set + .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - // Remove versioning tag - existing_tagging - .tag_set - .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - - let now = time::OffsetDateTime::now_utc(); - - if existing_tagging.tag_set.is_empty() { - // No tags remain; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize remaining tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } @@ -1050,65 +998,37 @@ pub async fn set_container_acl( } })?; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + // Rewrite the persisted tags: replace the ACL tags with the new grants + // while preserving all other tags. An empty result clears the tagging + // config. + update_swift_bucket_tagging(bucket_name, |current| { + let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); + tagging + .tag_set + .retain(|tag| tag.key.as_deref() != Some("swift-acl-read") && tag.key.as_deref() != Some("swift-acl-write")); - // Get existing tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); + if let Some(read) = read_acl + && !read.trim().is_empty() + { + tagging.tag_set.push(Tag { + key: Some("swift-acl-read".to_string()), + value: Some(read.to_string()), + }); + } - // Remove old ACL tags - existing_tagging - .tag_set - .retain(|tag| tag.key.as_deref() != Some("swift-acl-read") && tag.key.as_deref() != Some("swift-acl-write")); + if let Some(write) = write_acl + && !write.trim().is_empty() + { + tagging.tag_set.push(Tag { + key: Some("swift-acl-write".to_string()), + value: Some(write.to_string()), + }); + } - // Add new read ACL tag if provided - if let Some(read) = read_acl - && !read.trim().is_empty() - { - existing_tagging.tag_set.push(Tag { - key: Some("swift-acl-read".to_string()), - value: Some(read.to_string()), - }); - } - - // Add new write ACL tag if provided - if let Some(write) = write_acl - && !write.trim().is_empty() - { - existing_tagging.tag_set.push(Tag { - key: Some("swift-acl-write".to_string()), - value: Some(write.to_string()), - }); - } - - let now = time::OffsetDateTime::now_utc(); - - if existing_tagging.tag_set.is_empty() { - // No tags remain; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; debug!( "Set ACLs for container {}/{}: read={:?}, write={:?}", diff --git a/crates/protocols/src/swift/mod.rs b/crates/protocols/src/swift/mod.rs index 2dba11b8d..881b8fd4f 100644 --- a/crates/protocols/src/swift/mod.rs +++ b/crates/protocols/src/swift/mod.rs @@ -58,10 +58,53 @@ pub mod versioning; pub use errors::{SwiftError, SwiftResult}; pub use router::{SwiftRoute, SwiftRouter}; + +/// Maximum number of metadata headers allowed per resource (Swift standard) +pub(crate) const MAX_METADATA_COUNT: usize = 90; + +/// Maximum size in bytes for a single metadata value (Swift standard) +pub(crate) const MAX_METADATA_VALUE_SIZE: usize = 256; + +/// Validate metadata against Swift limits +/// +/// Checks that: +/// - Total number of metadata entries doesn't exceed MAX_METADATA_COUNT +/// - Individual metadata values don't exceed MAX_METADATA_VALUE_SIZE +/// +/// Applies to object, container and account metadata alike: all three are +/// persisted, and container/account metadata additionally lands in the +/// bucket metadata file that every later config write rewrites in full. +/// +/// Returns error if limits are exceeded. +pub(crate) fn validate_metadata(metadata: &std::collections::HashMap) -> SwiftResult<()> { + // Check total metadata count + if metadata.len() > MAX_METADATA_COUNT { + return Err(SwiftError::BadRequest(format!( + "Too many metadata headers: {} (max: {})", + metadata.len(), + MAX_METADATA_COUNT + ))); + } + + // Check individual value sizes + for (key, value) in metadata.iter() { + if value.len() > MAX_METADATA_VALUE_SIZE { + return Err(SwiftError::BadRequest(format!( + "Metadata value for '{}' too large: {} bytes (max: {} bytes)", + key, + value.len(), + MAX_METADATA_VALUE_SIZE + ))); + } + } + + Ok(()) +} + // Note: Container, Object, and SwiftMetadata types used by Swift implementation pub use storage_api::public_api::{SwiftGetObjectReader, SwiftObjectInfo, SwiftObjectOptions, SwiftPutObjReader}; pub(crate) use storage_api::public_api::{ - get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, set_swift_bucket_metadata, + get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging, }; #[allow(unused_imports)] pub use types::{Container, Object, SwiftMetadata}; diff --git a/crates/protocols/src/swift/object.rs b/crates/protocols/src/swift/object.rs index 66fa370c3..7620c5bd1 100644 --- a/crates/protocols/src/swift/object.rs +++ b/crates/protocols/src/swift/object.rs @@ -53,7 +53,7 @@ use super::account::validate_account_access; use super::container::ContainerMapper; use super::expiration_worker::{track_object_expiration, untrack_object_expiration}; use super::storage_api::object::{BucketOperations, BucketOptions, HTTPRangeSpec, ObjectIO as _, ObjectOperations as _}; -use super::{SwiftError, SwiftResult, resolve_swift_object_store_handle}; +use super::{SwiftError, SwiftResult, resolve_swift_object_store_handle, validate_metadata}; use axum::http::HeaderMap; use rustfs_credentials::Credentials; use rustfs_rio::HashReader; @@ -68,12 +68,6 @@ const LOG_SUBSYSTEM_SWIFT_OBJECT: &str = "swift_object"; const EVENT_SWIFT_OBJECT_STORAGE_STATE: &str = "swift_object_storage_state"; const SWIFT_DELETE_AT_METADATA: &str = "x-delete-at"; -/// Maximum number of metadata headers allowed per object (Swift standard) -const MAX_METADATA_COUNT: usize = 90; - -/// Maximum size in bytes for a single metadata value (Swift standard) -const MAX_METADATA_VALUE_SIZE: usize = 256; - /// Maximum object size in bytes (5GB - Swift default) const MAX_OBJECT_SIZE: i64 = 5 * 1024 * 1024 * 1024; @@ -234,38 +228,6 @@ impl Default for ObjectKeyMapper { } } -/// Validate metadata against Swift limits -/// -/// Checks that: -/// - Total number of metadata entries doesn't exceed MAX_METADATA_COUNT -/// - Individual metadata values don't exceed MAX_METADATA_VALUE_SIZE -/// -/// Returns error if limits are exceeded. -fn validate_metadata(metadata: &HashMap) -> SwiftResult<()> { - // Check total metadata count - if metadata.len() > MAX_METADATA_COUNT { - return Err(SwiftError::BadRequest(format!( - "Too many metadata headers: {} (max: {})", - metadata.len(), - MAX_METADATA_COUNT - ))); - } - - // Check individual value sizes - for (key, value) in metadata.iter() { - if value.len() > MAX_METADATA_VALUE_SIZE { - return Err(SwiftError::BadRequest(format!( - "Metadata value for '{}' too large: {} bytes (max: {} bytes)", - key, - value.len(), - MAX_METADATA_VALUE_SIZE - ))); - } - } - - Ok(()) -} - fn metadata_delete_at(metadata: &HashMap) -> Option { metadata .get(SWIFT_DELETE_AT_METADATA) diff --git a/crates/protocols/src/swift/storage_api.rs b/crates/protocols/src/swift/storage_api.rs index b65c711e5..70e7b052f 100644 --- a/crates/protocols/src/swift/storage_api.rs +++ b/crates/protocols/src/swift/storage_api.rs @@ -15,14 +15,19 @@ use std::collections::HashMap; use std::sync::Arc; +use rustfs_ecstore::api::bucket::metadata::BUCKET_TAGGING_CONFIG; pub(crate) use rustfs_ecstore::api::bucket::metadata::BucketMetadata as SwiftBucketMetadata; -use rustfs_ecstore::api::bucket::metadata_sys::{ - get as get_swift_bucket_metadata_from_backend, set_bucket_metadata as set_swift_bucket_metadata_in_backend, -}; +use rustfs_ecstore::api::bucket::metadata_sys::{get as get_swift_bucket_metadata_from_backend, update_config_with}; +use rustfs_ecstore::api::bucket::utils::serialize as serialize_bucket_config; +use rustfs_ecstore::api::error::Error as SwiftStorageError; pub(crate) use rustfs_ecstore::api::error::Result as SwiftStorageResult; +use rustfs_ecstore::api::notification::get_global_notification_sys; pub(crate) use rustfs_ecstore::api::runtime::object_store_handle as resolve_swift_object_store_handle; use rustfs_ecstore::api::storage::ECStore as SwiftStore; use rustfs_storage_api as storage_contracts; +use s3s::dto::Tagging; + +use super::{SwiftError, SwiftResult}; pub(crate) mod account { pub(crate) use super::storage_contracts::{BucketOperations, MakeBucketOptions}; @@ -45,7 +50,7 @@ pub(crate) mod object { pub(crate) mod public_api { pub use super::{SwiftGetObjectReader, SwiftObjectInfo, SwiftObjectOptions, SwiftPutObjReader}; pub(crate) use super::{ - get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, set_swift_bucket_metadata, + get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging, }; } @@ -53,6 +58,15 @@ pub(crate) mod versioning { pub(crate) use super::storage_contracts::{ListOperations, ObjectOperations}; } +const LOG_COMPONENT_PROTOCOLS: &str = "protocols"; +const LOG_SUBSYSTEM_SWIFT_STORAGE: &str = "swift_storage"; +const EVENT_SWIFT_BUCKET_TAGGING_UPDATE: &str = "swift_bucket_tagging_update"; + +/// Marks the refusal to rewrite an unreadable persisted tagging config, so the +/// caller can turn it into an actionable client error rather than a generic +/// storage failure. Carried through the ecstore error, which is a string type. +const UNREADABLE_TAGGING_SENTINEL: &str = "swift: persisted tagging config could not be parsed"; + pub type SwiftGetObjectReader = ::GetObjectReader; pub type SwiftObjectInfo = ::ObjectInfo; pub type SwiftObjectOptions = ::ObjectOptions; @@ -62,8 +76,80 @@ pub(crate) async fn get_swift_bucket_metadata(bucket: &str) -> SwiftStorageResul get_swift_bucket_metadata_from_backend(bucket).await } -pub(crate) async fn set_swift_bucket_metadata(bucket: String, metadata: SwiftBucketMetadata) -> SwiftStorageResult<()> { - set_swift_bucket_metadata_in_backend(bucket, metadata).await +/// Rewrite the bucket's tagging config through the persisting +/// bucket-metadata path. +/// +/// `rewrite` sees the tag set currently persisted on disk (`None` when the +/// bucket has none) and returns the full replacement; an empty tag set +/// clears the config. The read-modify-write runs under the bucket metadata +/// system's write guard — serialized against every other config update — +/// and the result is written to the bucket metadata file before the cache +/// is refreshed, so a Swift metadata POST survives process restarts and +/// disk-truth reloads. Peers are then told to reload, matching what the S3 +/// handlers do after a config write. +/// +/// Storage failures are logged in full and reported to the client as a +/// generic error: these now carry real disk and quorum detail, which does not +/// belong in a Swift response body. The one exception is an unreadable +/// persisted config, which is reported specifically because the operator has +/// to act on it. +pub(crate) async fn update_swift_bucket_tagging(bucket: String, rewrite: F) -> SwiftResult<()> +where + F: FnOnce(Option<&Tagging>) -> Tagging + Send, +{ + let result = update_config_with(&bucket, BUCKET_TAGGING_CONFIG, |bm| { + // Merging onto an unparseable tag set would silently drop every tag + // the bucket has — including the container ACL and versioning tags — + // because the rewrite closures treat "no parsed tags" as "no tags". + // Refuse instead: the persisted config is intact, just unreadable. + if !bm.tagging_config_xml.is_empty() && bm.tagging_config.is_none() { + return Err(SwiftStorageError::other(UNREADABLE_TAGGING_SENTINEL)); + } + + let tagging = rewrite(bm.tagging_config.as_ref()); + if tagging.tag_set.is_empty() { + Ok(Vec::new()) + } else { + // The S3 XML serializer, not quick_xml: the metadata loader's + // parse step must be able to round-trip what we persist. + serialize_bucket_config(&tagging) + .map_err(|e| SwiftStorageError::other(format!("failed to serialize bucket tagging: {e}"))) + } + }) + .await; + + if let Err(err) = result { + let unreadable = err.to_string().contains(UNREADABLE_TAGGING_SENTINEL); + tracing::error!( + event = EVENT_SWIFT_BUCKET_TAGGING_UPDATE, + component = LOG_COMPONENT_PROTOCOLS, + subsystem = LOG_SUBSYSTEM_SWIFT_STORAGE, + bucket = %bucket, + error = %err, + reason = if unreadable { "unreadable_persisted_config" } else { "storage_failure" }, + result = "failed", + "swift bucket tagging update failed" + ); + // A Swift-only client has no way to repair this itself, so say what + // happened and name the remedy instead of a bare storage error. + return Err(if unreadable { + SwiftError::Conflict(format!( + "The persisted tagging configuration for container store '{bucket}' cannot be parsed, so metadata cannot be updated without discarding it. Reset it with the S3 DeleteBucketTagging API." + )) + } else { + SwiftError::InternalServerError("Metadata update operation failed".to_string()) + }); + } + + if let Some(notification_sys) = get_global_notification_sys() { + tokio::spawn(async move { + if let Err(err) = notification_sys.load_bucket_metadata(&bucket).await { + tracing::warn!(bucket = %bucket, error = %err, "failed to notify peers after swift bucket tagging update"); + } + }); + } + + Ok(()) } pub(crate) async fn get_swift_bucket_usage() -> SwiftStorageResult>> { diff --git a/crates/protocols/tests/ecstore_test_compat/mod.rs b/crates/protocols/tests/ecstore_test_compat/mod.rs new file mode 100644 index 000000000..ebec36890 --- /dev/null +++ b/crates/protocols/tests/ecstore_test_compat/mod.rs @@ -0,0 +1,25 @@ +// Copyright 2024 RustFS Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! ECStore facade boundary for the protocols integration tests. +//! +//! The Swift tests need to drive bucket-metadata reloads the way the peer +//! `LoadBucketMetadata` RPC does. Everything they touch from `rustfs_ecstore` +//! is aliased here so the tests themselves hold no raw facade subpaths, the +//! same boundary `crates/protocols/src/swift/storage_api.rs` provides for the +//! Swift implementation. + +pub use rustfs_ecstore::api::bucket::metadata::load_bucket_metadata; +pub use rustfs_ecstore::api::bucket::metadata_sys::{get as get_bucket_metadata, set_bucket_metadata}; +pub use rustfs_ecstore::api::runtime::object_store_handle; diff --git a/crates/protocols/tests/swift_metadata_persistence.rs b/crates/protocols/tests/swift_metadata_persistence.rs new file mode 100644 index 000000000..25a7e42ad --- /dev/null +++ b/crates/protocols/tests/swift_metadata_persistence.rs @@ -0,0 +1,312 @@ +// Copyright 2024 RustFS Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Regression tests: a Swift metadata POST must be persisted to the bucket +//! metadata file, not just the in-memory cache. The metadata has to survive +//! the disk-truth reloads performed by peer LoadBucketMetadata notifications +//! and the periodic refresh loop — and, transitively, a process restart. + +#![cfg(feature = "swift")] + +use std::collections::HashMap; + +use rustfs_credentials::Credentials; +use rustfs_protocols::swift::SwiftError; +use rustfs_protocols::swift::container::{ContainerMapper, update_container_metadata}; +use rustfs_protocols::swift::{account, container}; +use rustfs_test_utils::TestECStoreEnv; +use serde_json::json; +use sha2::{Digest, Sha256}; + +mod ecstore_test_compat; +use ecstore_test_compat::{get_bucket_metadata, load_bucket_metadata, object_store_handle, set_bucket_metadata}; + +fn keystone_credentials(project_id: &str) -> Credentials { + let mut claims = HashMap::new(); + claims.insert("keystone_project_id".to_string(), json!(project_id)); + claims.insert("keystone_roles".to_string(), json!(["member"])); + + Credentials { + access_key: "keystone:swift-test".to_string(), + claims: Some(claims), + ..Default::default() + } +} + +/// The account-metadata bucket name scheme from `swift::account` +/// (`swift-account-{sha256(account)[0..16]}`), mirrored here so the test can +/// reload that bucket's metadata from disk. +fn account_metadata_bucket_name(account: &str) -> String { + let mut hasher = Sha256::new(); + hasher.update(account.as_bytes()); + let hash = hex::encode(hasher.finalize()); + format!("swift-account-{}", &hash[0..16]) +} + +/// Replace the cached bucket metadata with what is actually on disk — the +/// same thing a peer LoadBucketMetadata notification or the periodic refresh +/// loop does. Before the fix this silently discarded every Swift metadata +/// POST, because those writes only ever touched the cache. +async fn reload_bucket_metadata_from_disk(bucket: &str) { + let store = object_store_handle().expect("test store should be published"); + let bm = load_bucket_metadata(store, bucket) + .await + .expect("bucket metadata should load from disk"); + set_bucket_metadata(bucket.to_string(), bm) + .await + .expect("reloaded metadata should install"); +} + +/// The `swift-meta-*` tags currently persisted for a container, keyed the way +/// a Swift client sees them. `get_container_metadata` would be the natural +/// reader, but it additionally requires a data-usage snapshot for the object +/// count and byte total, which a bare test store has none of — and that is +/// orthogonal to whether the metadata itself was persisted. +async fn persisted_container_metadata(bucket: &str) -> HashMap { + let bm = get_bucket_metadata(bucket).await.expect("bucket metadata should be cached"); + let mut out = HashMap::new(); + if let Some(tagging) = &bm.tagging_config { + for tag in &tagging.tag_set { + if let (Some(key), Some(value)) = (&tag.key, &tag.value) + && let Some(meta_key) = key.strip_prefix("swift-meta-") + { + out.insert(meta_key.to_string(), value.clone()); + } + } + } + out +} + +/// Every scenario that needs a store runs against ONE environment: the Swift +/// handlers resolve the ambient object store and bucket-metadata system, so a +/// second `TestECStoreEnv` in this process would race the first. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn swift_metadata_writes_are_durable() { + let env = TestECStoreEnv::builder().prefix("swift_meta_persist").build().await; + + posts_survive_disk_truth_reload(&env).await; + tag_writers_preserve_each_others_state(&env).await; + versioning_writes_reject_missing_containers().await; +} + +async fn posts_survive_disk_truth_reload(env: &TestECStoreEnv) { + // --- Container metadata POST (X-Container-Meta-*) --- + let project_id = "swiftpersistproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = keystone_credentials(project_id); + let swift_container = "photos"; + let bucket = ContainerMapper::default().swift_to_s3_bucket(swift_container, project_id); + env.make_bucket(&bucket, false).await; + + let mut metadata = HashMap::new(); + metadata.insert("color".to_string(), "blue".to_string()); + update_container_metadata(&swift_account, swift_container, &credentials, metadata) + .await + .expect("container metadata POST should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + let container_meta = persisted_container_metadata(&bucket).await; + assert_eq!( + container_meta.get("color").map(String::as_str), + Some("blue"), + "container metadata POST must survive a disk-truth metadata reload" + ); + + // A follow-up POST replaces the Swift metadata and that replacement must + // survive a reload too (the rewrite merges against disk state, so the + // previous value must actually be gone). + let mut metadata = HashMap::new(); + metadata.insert("season".to_string(), "summer".to_string()); + update_container_metadata(&swift_account, swift_container, &credentials, metadata) + .await + .expect("second container metadata POST should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + let container_meta = persisted_container_metadata(&bucket).await; + assert_eq!(container_meta.get("season").map(String::as_str), Some("summer")); + assert!( + !container_meta.contains_key("color"), + "replaced container metadata must not resurrect on reload" + ); + + // --- Container versioning POST (X-Versions-Location) --- + let archive_container = "photos-archive"; + let archive_bucket = ContainerMapper::default().swift_to_s3_bucket(archive_container, project_id); + env.make_bucket(&archive_bucket, false).await; + + container::enable_versioning(&swift_account, swift_container, archive_container, &credentials) + .await + .expect("enable versioning should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + let location = container::get_versions_location(&swift_account, swift_container, &credentials) + .await + .expect("versions location should load"); + assert_eq!( + location.as_deref(), + Some(archive_container), + "versions location must survive a disk-truth metadata reload" + ); + + // --- Account metadata POST (TempURL keys etc.) --- + let mut account_meta = HashMap::new(); + account_meta.insert("temp-url-key".to_string(), "s3cr3t".to_string()); + account::update_account_metadata(&swift_account, &account_meta, &Some(credentials.clone())) + .await + .expect("account metadata POST should succeed"); + + reload_bucket_metadata_from_disk(&account_metadata_bucket_name(&swift_account)).await; + + let loaded = account::get_account_metadata(&swift_account, &None) + .await + .expect("account metadata should load"); + assert_eq!( + loaded.get("temp-url-key").map(String::as_str), + Some("s3cr3t"), + "account metadata POST must survive a disk-truth metadata reload" + ); +} + +/// The rewrites all share one tag set, so a closure that ignored the current +/// state would still pass a single-feature test. Drive ACLs, versioning and +/// container metadata over the same container and assert each survives the +/// others — and that clearing one leaves the rest alone. +async fn tag_writers_preserve_each_others_state(env: &TestECStoreEnv) { + let project_id = "swiftcrosstagproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = keystone_credentials(project_id); + let container = "shared"; + let archive = "shared-archive"; + let bucket = ContainerMapper::default().swift_to_s3_bucket(container, project_id); + env.make_bucket(&bucket, false).await; + env.make_bucket(&ContainerMapper::default().swift_to_s3_bucket(archive, project_id), false) + .await; + + let mut metadata = HashMap::new(); + metadata.insert("color".to_string(), "blue".to_string()); + update_container_metadata(&swift_account, container, &credentials, metadata) + .await + .expect("container metadata POST should succeed"); + container::enable_versioning(&swift_account, container, archive, &credentials) + .await + .expect("enable versioning should succeed"); + container::set_container_acl(&swift_account, container, Some(".r:*"), Some("AUTH_other"), &credentials) + .await + .expect("set container ACL should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + // All three writers' state coexists after a disk-truth reload. + let meta = persisted_container_metadata(&bucket).await; + assert_eq!(meta.get("color").map(String::as_str), Some("blue")); + assert_eq!( + container::get_versions_location(&swift_account, container, &credentials) + .await + .expect("versions location should load") + .as_deref(), + Some(archive) + ); + let acl = container::get_container_acl(&swift_account, container, &credentials) + .await + .expect("container ACL should load"); + assert!(!acl.read.is_empty(), "read ACL must survive the reload"); + assert!(!acl.write.is_empty(), "write ACL must survive the reload"); + + // Disabling versioning drops only the versioning tag. + container::disable_versioning(&swift_account, container, &credentials) + .await + .expect("disable versioning should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + assert_eq!( + container::get_versions_location(&swift_account, container, &credentials) + .await + .expect("versions location should load"), + None, + "disable_versioning must clear the versioning tag durably" + ); + let meta = persisted_container_metadata(&bucket).await; + assert_eq!( + meta.get("color").map(String::as_str), + Some("blue"), + "disable_versioning must not disturb container metadata" + ); + let acl = container::get_container_acl(&swift_account, container, &credentials) + .await + .expect("container ACL should load"); + assert!(!acl.read.is_empty(), "disable_versioning must not disturb the ACL"); +} + +/// A container that does not exist must not get metadata persisted for it: +/// the metadata loader turns "nothing on disk" into a fresh default, so an +/// unguarded rewrite would create an orphan metadata file and cache a +/// fabricated default as authoritative. +async fn versioning_writes_reject_missing_containers() { + let project_id = "swiftmissingproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = keystone_credentials(project_id); + let missing = "no-such-container"; + + let err = container::disable_versioning(&swift_account, missing, &credentials) + .await + .expect_err("disabling versioning on a missing container must fail"); + assert!( + matches!(err, SwiftError::NotFound(_)), + "expected NotFound for a missing container, got {err:?}" + ); + + let bucket = ContainerMapper::default().swift_to_s3_bucket(missing, project_id); + assert!( + get_bucket_metadata(&bucket).await.is_err(), + "a rejected write must not have cached metadata for a nonexistent container" + ); +} + +/// Account metadata holds the account's TempURL signing key, and it is now +/// durable — so a write for someone else's account would be a persistent, +/// cluster-wide takeover of that account's pre-signed URLs, not a cache blip. +/// The write path must reject both a foreign account and a missing token, +/// while reads stay open for pre-auth TempURL signature validation. +/// +/// Deliberately builds no store: both rejections must happen before the write +/// path resolves storage at all, and a second `TestECStoreEnv` in this process +/// would race the other test over the ambient store handle. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn account_metadata_write_rejects_foreign_and_anonymous_callers() { + let victim_account = "AUTH_victimproject"; + let attacker_credentials = keystone_credentials("attackerproject"); + + let mut poisoned = HashMap::new(); + poisoned.insert("temp-url-key".to_string(), "attacker-key".to_string()); + + let err = account::update_account_metadata(victim_account, &poisoned, &Some(attacker_credentials)) + .await + .expect_err("writing another account's metadata must be rejected"); + assert!( + matches!(err, SwiftError::Forbidden(_)), + "cross-account metadata write must be Forbidden, got {err:?}" + ); + + let err = account::update_account_metadata(victim_account, &poisoned, &None) + .await + .expect_err("anonymous account metadata write must be rejected"); + assert!( + matches!(err, SwiftError::Unauthorized(_)), + "anonymous metadata write must be Unauthorized, got {err:?}" + ); +} From 3d80578abd5ef0eab3597540d146dc5d0d0348c1 Mon Sep 17 00:00:00 2001 From: cxymds Date: Wed, 29 Jul 2026 10:42:35 +0800 Subject: [PATCH 5/8] feat(ecstore): add local data-dir snapshot leases (#5388) feat(ecstore): add local snapshot leases Co-authored-by: houseme --- crates/ecstore/src/disk/disk_store.rs | 30 +- crates/ecstore/src/disk/local.rs | 340 ++++++++++++++++-- crates/ecstore/src/disk/mod.rs | 52 +++ .../src/set_disk/core/io_primitives.rs | 103 +++++- 4 files changed, 479 insertions(+), 46 deletions(-) diff --git a/crates/ecstore/src/disk/disk_store.rs b/crates/ecstore/src/disk/disk_store.rs index 01171b8f1..940638853 100644 --- a/crates/ecstore/src/disk/disk_store.rs +++ b/crates/ecstore/src/disk/disk_store.rs @@ -13,9 +13,9 @@ // limitations under the License. use crate::disk::{ - CheckPartsResp, DeleteOptions, DiskAPI, DiskError, DiskInfo, DiskInfoOptions, DiskLocation, Endpoint, Error, - FileInfoVersions, MmapCopyStageMetrics, ReadMultipleReq, ReadMultipleResp, ReadOptions, RenameDataResp, Result, - UpdateMetadataOpts, VolumeInfo, WalkDirOptions, + CheckPartsResp, DataDirDeleteStatus, DeleteOptions, DiskAPI, DiskError, DiskInfo, DiskInfoOptions, DiskLocation, Endpoint, + Error, FileInfoVersions, MmapCopyStageMetrics, ReadMultipleReq, ReadMultipleResp, ReadOptions, RenameDataResp, Result, + SnapshotLeaseToken, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, health_state::{ RuntimeDriveHealthState, classify_drive_recovery, get_drive_returning_probe_interval, get_drive_returning_success_threshold, get_drive_suspect_failure_threshold, record_drive_offline_duration, @@ -1349,6 +1349,30 @@ impl DiskAPI for LocalDiskWrapper { .await } + async fn acquire_snapshot_lease(&self, volume: &str, path: &str) -> Result { + self.track_disk_health( + || async { self.disk.acquire_snapshot_lease(volume, path).await }, + get_max_timeout_duration(), + ) + .await + } + + async fn release_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result<()> { + self.track_disk_health( + || async { self.disk.release_snapshot_lease(volume, path, token).await }, + get_max_timeout_duration(), + ) + .await + } + + async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { + self.track_disk_health( + || async { self.disk.delete_data_dir(volume, path, opts).await }, + get_max_timeout_duration(), + ) + .await + } + async fn write_metadata(&self, org_volume: &str, volume: &str, path: &str, fi: FileInfo) -> Result<()> { self.track_disk_health( || async { self.disk.write_metadata(org_volume, volume, path, fi).await }, diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index c24e38385..991a2248b 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -18,11 +18,12 @@ use crate::data_usage::local_snapshot::ensure_data_usage_layout; use crate::disk::disk_store::{get_drive_walkdir_stall_timeout, get_object_disk_read_timeout}; use crate::disk::{ BUCKET_META_PREFIX, CHECK_PART_FILE_CORRUPT, CHECK_PART_FILE_NOT_FOUND, CHECK_PART_SUCCESS, CHECK_PART_UNKNOWN, - CHECK_PART_VOLUME_NOT_FOUND, CheckPartsResp, DeleteOptions, DiskAPI, DiskInfo, DiskInfoOptions, DiskLocation, DiskMetrics, - FileInfoVersions, FileReader, FileWriter, MmapCopyStageMetrics, OldCurrentSize, PART_TRANSACTION_NEW_META, - PART_TRANSACTION_OLD_META, PART_TRANSACTION_ROLLBACK, PartTransactionAction, RUSTFS_META_BUCKET, RUSTFS_META_TMP_BUCKET, - RUSTFS_META_TMP_DELETED_BUCKET, ReadMultipleReq, ReadMultipleResp, ReadOptions, RenameDataResp, STORAGE_FORMAT_FILE, - STORAGE_FORMAT_FILE_BACKUP, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, conv_part_err_to_int, + CHECK_PART_VOLUME_NOT_FOUND, CheckPartsResp, DataDirDeleteStatus, DeleteOptions, DiskAPI, DiskInfo, DiskInfoOptions, + DiskLocation, DiskMetrics, FileInfoVersions, FileReader, FileWriter, MmapCopyStageMetrics, OldCurrentSize, + PART_TRANSACTION_NEW_META, PART_TRANSACTION_OLD_META, PART_TRANSACTION_ROLLBACK, PartTransactionAction, RUSTFS_META_BUCKET, + RUSTFS_META_TMP_BUCKET, RUSTFS_META_TMP_DELETED_BUCKET, ReadMultipleReq, ReadMultipleResp, ReadOptions, RenameDataResp, + STORAGE_FORMAT_FILE, STORAGE_FORMAT_FILE_BACKUP, SnapshotLeaseToken, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, + conv_part_err_to_int, endpoint::Endpoint, error::{DiskError, Error, FileAccessDeniedWithContext, Result}, error_conv::{to_access_error, to_file_error, to_unformatted_disk_error, to_volume_error}, @@ -66,7 +67,7 @@ use tokio::fs::{self, File}; #[cfg(not(unix))] use tokio::io::AsyncReadExt; use tokio::io::{AsyncRead, AsyncSeekExt, AsyncWrite, AsyncWriteExt, ErrorKind, ReadBuf}; -use tokio::sync::{Notify, RwLock, Semaphore}; +use tokio::sync::{Mutex, Notify, RwLock, Semaphore}; use tokio::time::{Instant, Sleep, interval_at, timeout}; use tracing::{debug, error, info, warn}; use uuid::Uuid; @@ -3856,6 +3857,25 @@ pub struct LocalDisk { exit_signal: Option>, io_backend: Arc, file_sync_permits: Arc, + snapshot_leases: Arc>, +} + +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +struct SnapshotLeaseKey { + volume: String, + path: String, +} + +#[derive(Default)] +struct SnapshotLeaseEntry { + tokens: HashSet, + pending_delete: Option, + deleting: bool, +} + +#[derive(Default)] +struct SnapshotLeaseRegistry { + entries: HashMap, } impl Drop for LocalDisk { @@ -4092,6 +4112,7 @@ impl LocalDisk { exit_signal: None, io_backend: build_local_io_backend(root.clone()), file_sync_permits: os::disk_file_sync_limiter(&root), + snapshot_leases: Arc::new(Mutex::new(SnapshotLeaseRegistry::default())), }; let (info, _root) = get_disk_info(root.clone()).await.inspect_err(|err| { log_startup_disk_error("get_disk_info", &root, err); @@ -4576,6 +4597,23 @@ impl LocalDisk { Ok(()) } + async fn delete_unleased(&self, volume: &str, path: &str, opt: &DeleteOptions) -> Result<()> { + let volume_dir = self.get_bucket_path(volume)?; + if !skip_access_checks(volume) + && let Err(e) = access(&volume_dir).await + { + return Err(to_access_error(e, DiskError::VolumeAccessDenied).into()); + } + + let file_path = self.get_object_path(volume, path)?; + check_path_length(file_path.to_string_lossy().as_ref())?; + self.delete_file(&volume_dir, &file_path, opt.recursive, opt.immediate) + .await?; + // A deleted shard must not remain readable through the io_uring fd cache. + self.io_backend.invalidate_cached_fds_under(volume, path); + Ok(()) + } + #[tracing::instrument(level = "trace", skip_all)] #[async_recursion::async_recursion] async fn delete_file( @@ -6216,27 +6254,7 @@ impl DiskAPI for LocalDisk { #[tracing::instrument(level = "trace", skip_all)] async fn delete(&self, volume: &str, path: &str, opt: DeleteOptions) -> Result<()> { crate::hp_guard!("LocalDisk::delete"); - let volume_dir = self.get_bucket_path(volume)?; - if !skip_access_checks(volume) - && let Err(e) = access(&volume_dir).await - { - return Err(to_access_error(e, DiskError::VolumeAccessDenied).into()); - } - - let file_path = self.get_object_path(volume, path)?; - - check_path_length(file_path.to_string_lossy().to_string().as_str())?; - - self.delete_file(&volume_dir, &file_path, opt.recursive, opt.immediate) - .await?; - - // The inode is unlinked, but a cached descriptor would keep it readable — - // a deleted shard must not keep answering reads (backlog#1145). The part - // numbers under `path` are not known here, so this is the one caller that - // needs the predicate form. - self.io_backend.invalidate_cached_fds_under(volume, path); - - Ok(()) + self.delete_unleased(volume, path, &opt).await } #[tracing::instrument(level = "trace", skip_all)] @@ -7824,6 +7842,118 @@ impl DiskAPI for LocalDisk { Ok(()) } + async fn acquire_snapshot_lease(&self, volume: &str, path: &str) -> Result { + let file_path = self.get_object_path(volume, path)?; + let key = SnapshotLeaseKey { + volume: volume.to_string(), + path: path.to_string(), + }; + let token = { + let mut registry = self.snapshot_leases.lock().await; + if registry.entries.get(&key).is_some_and(|entry| entry.deleting) { + return Err(DiskError::FileNotFound); + } + let token = SnapshotLeaseToken::new(); + registry.entries.entry(key).or_default().tokens.insert(token); + token + }; + match fs::metadata(file_path).await { + Ok(metadata) if metadata.is_dir() => Ok(token), + Ok(_) => { + self.release_snapshot_lease(volume, path, token).await?; + Err(DiskError::FileNotFound) + } + Err(err) => { + self.release_snapshot_lease(volume, path, token).await?; + Err(to_file_error(err).into()) + } + } + } + + async fn release_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result<()> { + let key = SnapshotLeaseKey { + volume: volume.to_string(), + path: path.to_string(), + }; + let opts = { + let mut registry = self.snapshot_leases.lock().await; + let Some(entry) = registry.entries.get_mut(&key) else { + return Ok(()); + }; + entry.tokens.remove(&token); + if !entry.tokens.is_empty() || entry.deleting { + return Ok(()); + } + + let Some(opts) = entry.pending_delete.clone() else { + registry.entries.remove(&key); + return Ok(()); + }; + entry.deleting = true; + opts + }; + let result = self.delete_unleased(volume, path, &opts).await; + let mut registry = self.snapshot_leases.lock().await; + match result { + Ok(()) => { + registry.entries.remove(&key); + Ok(()) + } + Err(err) => { + if let Some(entry) = registry.entries.get_mut(&key) { + entry.deleting = false; + } + Err(err) + } + } + } + + async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { + let key = SnapshotLeaseKey { + volume: volume.to_string(), + path: path.to_string(), + }; + { + let mut registry = self.snapshot_leases.lock().await; + if let Some(entry) = registry.entries.get_mut(&key) { + if !entry.tokens.is_empty() { + entry.pending_delete.get_or_insert_with(|| opts.clone()); + return Ok(DataDirDeleteStatus::Deferred); + } + if entry.deleting { + entry.pending_delete.get_or_insert_with(|| opts.clone()); + return Ok(DataDirDeleteStatus::Deferred); + } + entry.deleting = true; + entry.pending_delete.get_or_insert_with(|| opts.clone()); + } else { + registry.entries.insert( + key.clone(), + SnapshotLeaseEntry { + pending_delete: Some(opts.clone()), + deleting: true, + ..Default::default() + }, + ); + } + } + + let result = self.delete_unleased(volume, path, &opts).await; + let mut registry = self.snapshot_leases.lock().await; + match result { + Ok(()) => { + registry.entries.remove(&key); + Ok(DataDirDeleteStatus::Deleted) + } + Err(err) => { + if let Some(entry) = registry.entries.get_mut(&key) { + entry.deleting = false; + } + Err(err) + } + } + } + #[tracing::instrument(level = "trace", skip_all)] async fn update_metadata(&self, volume: &str, path: &str, fi: FileInfo, opts: &UpdateMetadataOpts) -> Result<()> { if !fi.metadata.is_empty() { @@ -14800,6 +14930,162 @@ mod test { assert!(construction_source.is::()); } + #[tokio::test] + async fn snapshot_leases_defer_data_dir_cleanup_until_last_release() { + use tempfile::tempdir; + + let root_dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(root_dir.path().to_string_lossy().as_ref()).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let volume = "snapshot-lease-volume"; + let data_dir = path_join_buf(&["object", &Uuid::new_v4().to_string()]); + let first_part = path_join_buf(&[&data_dir, "part.1"]); + let later_part = path_join_buf(&[&data_dir, "part.2"]); + ensure_test_volume(&disk, volume).await; + disk.write_all(volume, &first_part, Bytes::from_static(b"first")) + .await + .expect("first shard should be written"); + disk.write_all(volume, &later_part, Bytes::from_static(b"later")) + .await + .expect("later shard should be written"); + + let first = disk + .acquire_snapshot_lease(volume, &data_dir) + .await + .expect("first lease should be acquired"); + let second = disk + .acquire_snapshot_lease(volume, &data_dir) + .await + .expect("second lease should be acquired"); + let status = disk + .delete_data_dir( + volume, + &data_dir, + DeleteOptions { + recursive: true, + ..Default::default() + }, + ) + .await + .expect("cleanup should be deferred"); + assert_eq!(status, DataDirDeleteStatus::Deferred); + assert_eq!( + disk.read_all(volume, &later_part) + .await + .expect("a later multipart shard must remain openable while leased"), + Bytes::from_static(b"later") + ); + + disk.release_snapshot_lease(volume, &data_dir, first) + .await + .expect("first lease release should succeed"); + assert!( + disk.read_all(volume, &first_part).await.is_ok(), + "one remaining lease must keep the data directory" + ); + disk.release_snapshot_lease(volume, &data_dir, second) + .await + .expect("last lease release should run deferred cleanup"); + disk.release_snapshot_lease(volume, &data_dir, second) + .await + .expect("releasing an already released token should be idempotent"); + assert!(matches!(disk.read_all(volume, &first_part).await, Err(DiskError::FileNotFound))); + } + + #[tokio::test] + async fn data_dir_cleanup_without_a_lease_keeps_existing_behavior() { + use tempfile::tempdir; + + let root_dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(root_dir.path().to_string_lossy().as_ref()).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let volume = "snapshot-no-lease-volume"; + let data_dir = path_join_buf(&["object", &Uuid::new_v4().to_string()]); + let part = path_join_buf(&[&data_dir, "part.1"]); + ensure_test_volume(&disk, volume).await; + disk.write_all(volume, &part, Bytes::from_static(b"payload")) + .await + .expect("test shard should be written"); + + let status = disk + .delete_data_dir( + volume, + &data_dir, + DeleteOptions { + recursive: true, + ..Default::default() + }, + ) + .await + .expect("unleased cleanup should retain the existing delete behavior"); + assert_eq!(status, DataDirDeleteStatus::Deleted); + assert!(matches!(disk.read_all(volume, &part).await, Err(DiskError::FileNotFound))); + } + + #[tokio::test] + async fn snapshot_lease_acquire_and_cleanup_are_atomic() { + use tempfile::tempdir; + + let root_dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(root_dir.path().to_string_lossy().as_ref()).expect("endpoint should parse"); + let disk = Arc::new(LocalDisk::new(&endpoint, false).await.expect("local disk should be created")); + let volume = "snapshot-race-volume"; + ensure_test_volume(&disk, volume).await; + + for iteration in 0..32 { + let data_dir = path_join_buf(&["object", &format!("{iteration:032x}")]); + let part = path_join_buf(&[&data_dir, "part.1"]); + disk.write_all(volume, &part, Bytes::from_static(b"payload")) + .await + .expect("test shard should be written"); + let barrier = Arc::new(tokio::sync::Barrier::new(3)); + let acquire_disk = Arc::clone(&disk); + let acquire_barrier = Arc::clone(&barrier); + let acquire_path = data_dir.clone(); + let acquire = tokio::spawn(async move { + acquire_barrier.wait().await; + acquire_disk.acquire_snapshot_lease(volume, &acquire_path).await + }); + let delete_disk = Arc::clone(&disk); + let delete_barrier = Arc::clone(&barrier); + let delete_path = data_dir.clone(); + let delete = tokio::spawn(async move { + delete_barrier.wait().await; + delete_disk + .delete_data_dir( + volume, + &delete_path, + DeleteOptions { + recursive: true, + ..Default::default() + }, + ) + .await + }); + barrier.wait().await; + + let acquired = acquire.await.expect("acquire task should join"); + let deleted = delete + .await + .expect("delete task should join") + .expect("delete should either run or defer"); + match acquired { + Ok(token) => { + assert_eq!(deleted, DataDirDeleteStatus::Deferred); + assert!(disk.read_all(volume, &part).await.is_ok()); + disk.release_snapshot_lease(volume, &data_dir, token) + .await + .expect("release should finish deferred cleanup"); + } + Err(DiskError::FileNotFound) => { + assert_eq!(deleted, DataDirDeleteStatus::Deleted); + } + Err(err) => panic!("unexpected lease acquisition error: {err}"), + } + assert!(matches!(disk.read_all(volume, &part).await, Err(DiskError::FileNotFound))); + } + } + #[tokio::test] async fn local_disk_check_parts_rejects_zero_data_geometry_before_shard_math() { use tempfile::tempdir; diff --git a/crates/ecstore/src/disk/mod.rs b/crates/ecstore/src/disk/mod.rs index fc17998e8..ee947d291 100644 --- a/crates/ecstore/src/disk/mod.rs +++ b/crates/ecstore/src/disk/mod.rs @@ -72,6 +72,27 @@ pub type DiskStore = Arc; pub type FileReader = Box; pub type FileWriter = Box; +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)] +pub struct SnapshotLeaseToken(Uuid); + +impl SnapshotLeaseToken { + pub fn new() -> Self { + Self(Uuid::new_v4()) + } +} + +impl Default for SnapshotLeaseToken { + fn default() -> Self { + Self::new() + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum DataDirDeleteStatus { + Deleted, + Deferred, +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum PartTransactionAction { Commit, @@ -249,6 +270,27 @@ impl DiskAPI for Disk { } } + async fn acquire_snapshot_lease(&self, volume: &str, path: &str) -> Result { + match self { + Disk::Local(local_disk) => local_disk.acquire_snapshot_lease(volume, path).await, + Disk::Remote(remote_disk) => remote_disk.acquire_snapshot_lease(volume, path).await, + } + } + + async fn release_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result<()> { + match self { + Disk::Local(local_disk) => local_disk.release_snapshot_lease(volume, path, token).await, + Disk::Remote(remote_disk) => remote_disk.release_snapshot_lease(volume, path, token).await, + } + } + + async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { + match self { + Disk::Local(local_disk) => local_disk.delete_data_dir(volume, path, opts).await, + Disk::Remote(remote_disk) => remote_disk.delete_data_dir(volume, path, opts).await, + } + } + #[tracing::instrument(level = "trace", skip_all)] async fn write_metadata(&self, _org_volume: &str, volume: &str, path: &str, fi: FileInfo) -> Result<()> { match self { @@ -646,6 +688,16 @@ pub trait DiskAPI: Debug + Send + Sync + 'static { ) -> Result<()>; async fn delete_versions(&self, volume: &str, versions: Vec, opts: DeleteOptions) -> Vec>; async fn delete_paths(&self, volume: &str, paths: &[String]) -> Result<()>; + async fn acquire_snapshot_lease(&self, _volume: &str, _path: &str) -> Result { + Err(Error::other("snapshot leases are not supported by this disk")) + } + async fn release_snapshot_lease(&self, _volume: &str, _path: &str, _token: SnapshotLeaseToken) -> Result<()> { + Err(Error::other("snapshot leases are not supported by this disk")) + } + async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { + self.delete(volume, path, opts).await?; + Ok(DataDirDeleteStatus::Deleted) + } async fn write_metadata(&self, org_volume: &str, volume: &str, path: &str, fi: FileInfo) -> Result<()>; async fn update_metadata(&self, volume: &str, path: &str, fi: FileInfo, opts: &UpdateMetadataOpts) -> Result<()>; async fn read_version( diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index dd5a9b5b5..4972274f1 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -47,8 +47,8 @@ use crate::diagnostics::get::{ record_get_object_pipeline_failure_for_path, record_get_stage_duration_if_enabled, }; use crate::disk::{ - OldCurrentSize, PART_TRANSACTION_NEW_META, PART_TRANSACTION_OLD_META, PART_TRANSACTION_ROLLBACK, PartTransactionAction, - part_transaction_path, + DataDirDeleteStatus, OldCurrentSize, PART_TRANSACTION_NEW_META, PART_TRANSACTION_OLD_META, PART_TRANSACTION_ROLLBACK, + PartTransactionAction, part_transaction_path, }; use crate::erasure::coding::BitrotReader; use crate::io_support::bitrot::ShardReader; @@ -2985,29 +2985,48 @@ impl SetDisks { Self::rename_fanout_barrier(&object_for_fault, idx, rename_fanout_barrier_phase::CLEANUP).await; if let Some(err) = Self::cleanup_injected_error(&object_for_fault, idx) { - return Some(err); + return (false, Some(err)); } if let Some(disk) = disk { - disk.delete( - &bucket, - &file_path, - DeleteOptions { - recursive: true, - ..Default::default() - }, - ) - .await - .err() + match disk + .delete_data_dir( + &bucket, + &file_path, + DeleteOptions { + recursive: true, + ..Default::default() + }, + ) + .await + { + Ok(DataDirDeleteStatus::Deleted) => (false, None), + Ok(DataDirDeleteStatus::Deferred) => (true, None), + Err(err) => (false, Some(err)), + } } else { // `None` slot: ignored placeholder. It is not `attempted`, so // classification excludes it from residue regardless. - Some(DiskError::DiskNotFound) + (false, Some(DiskError::DiskNotFound)) } }) }); - let errs: Vec> = join_all(futures).await.into_iter().map(map_cleanup_join_result).collect(); + let mut deferred = 0usize; + let errs: Vec> = join_all(futures) + .await + .into_iter() + .map(|result| match result { + Ok((was_deferred, err)) => { + deferred += usize::from(was_deferred); + err + } + Err(join_err) => Some(DiskError::other(format!("old data dir cleanup task failed: {join_err}"))), + }) + .collect(); - classify_old_data_dir_cleanup(&errs, &attempted, write_quorum) + let mut cleanup = classify_old_data_dir_cleanup(&errs, &attempted, write_quorum); + cleanup.deferred = deferred; + cleanup.reclaimed = cleanup.reclaimed.saturating_sub(deferred); + cleanup } /// Test-only fault-injection seam for the old-data-dir cleanup path @@ -3098,6 +3117,20 @@ impl SetDisks { rustfs_io_metrics::record_old_data_dir_cleanup(c.attempted, c.reclaimed, c.unreclaimed_disks.len(), c.below_quorum); + if c.deferred > 0 { + debug!( + event = EVENT_SET_DISK_WRITE, + component = LOG_COMPONENT_ECSTORE, + subsystem = LOG_SUBSYSTEM_SET_DISK, + bucket = %bucket, + object = %object, + old_data_dir = %old_dir, + deferred = c.deferred, + state = "old_data_cleanup_deferred", + "Old data directory cleanup deferred for active snapshot leases" + ); + } + if actions.warn { warn!( component = LOG_COMPONENT_ECSTORE, @@ -4129,6 +4162,9 @@ pub(in crate::set_disk) struct OldDataDirCleanup { /// Number of attempted disks that returned `Ok` or a not-found variant /// (a missing dir == already reclaimed). pub reclaimed: usize, + /// Number of attempted disks that retained the directory for an active + /// snapshot lease and registered it for deletion after the final release. + pub deferred: usize, /// Indices of attempted disks that failed with a non-ignored, non-not-found /// error (including task panic/cancel). This is the residue that actually /// leaks and drives the leak metric + heal enqueue. @@ -4191,6 +4227,7 @@ fn classify_old_data_dir_cleanup(errs: &[Option], attempted: &[bool], OldDataDirCleanup { attempted: attempted_count, reclaimed, + deferred: 0, unreclaimed_disks, below_quorum, } @@ -5131,6 +5168,40 @@ mod tests { drop((disk1, disk2)); } + #[tokio::test] + async fn commit_cleanup_reports_and_releases_deferred_snapshot_data_dirs() { + let bucket = "cleanup-lease-bucket"; + let object = "cleanup-lease-object"; + let old_data_dir = "11111111-1111-1111-1111-111111111111"; + let committed_data_dir = "22222222-2222-2222-2222-222222222222"; + let data_dir_path = format!("{object}/{old_data_dir}"); + let shard_path = format!("{data_dir_path}/part.1"); + let (_dir1, disk1) = read_multiple_test_disk(bucket, &[(&shard_path, b"one".as_slice())]).await; + let set = io_primitives_test_set(vec![Some(disk1.clone())], 0).await; + let lease = disk1 + .acquire_snapshot_lease(bucket, &data_dir_path) + .await + .expect("snapshot lease should be acquired before cleanup"); + + let cleanup = set + .commit_rename_data_dir(&[Some(disk1.clone())], bucket, object, old_data_dir, committed_data_dir, 1) + .await; + assert_eq!(cleanup.attempted, 1); + assert_eq!(cleanup.reclaimed, 0); + assert_eq!(cleanup.deferred, 1); + assert!(cleanup.unreclaimed_disks.is_empty()); + disk1 + .read_all(bucket, &shard_path) + .await + .expect("deferred cleanup must leave later shard opens available"); + + disk1 + .release_snapshot_lease(bucket, &data_dir_path, lease) + .await + .expect("final lease release should reclaim the old data directory"); + assert!(matches!(disk1.read_all(bucket, &shard_path).await, Err(DiskError::FileNotFound))); + } + /// Isolation guard: an armed barrier / observed object only affects its own /// object. A fan-out for a different (unobserved, unarmed) object must not be /// paused and must not accrue any tracked task count — so concurrent tests From 294c79c156fbefb18996c4644a3149aaa4bfdaf6 Mon Sep 17 00:00:00 2001 From: cxymds Date: Wed, 29 Jul 2026 10:43:28 +0800 Subject: [PATCH 6/8] fix(tiering): gate remote version state safely (#5374) * feat(tiering): model provider version capabilities * feat(tiering): persist opaque remote versions * fix(tiering): gate remote version state safely * fix(tiering): preserve remote version state on delete * fix(tiering): accept unversioned transition responses * fix(tiering): replay exact cleanup journals * test(tiering): pin empty exact cleanup guard * test(tiering): accept strict missing journal errors * test(tiering): exercise free-version identity guard * test(tiering): reach destination identity guard * test(tiering): persist version identity drift * test(tiering): bind version drift fixture --------- Co-authored-by: houseme --- .../bucket/lifecycle/bucket_lifecycle_ops.rs | 191 ++++++++++- .../bucket/lifecycle/tier_delete_journal.rs | 169 ++++++++-- .../src/bucket/lifecycle/tier_sweeper.rs | 179 +++++++++- .../lifecycle/transition_transaction.rs | 20 +- crates/ecstore/src/config/com.rs | 1 + crates/ecstore/src/object_api/types.rs | 13 +- crates/ecstore/src/services/tier/test_util.rs | 5 +- crates/ecstore/src/services/tier/tier.rs | 3 +- .../src/set_disk/core/io_primitives.rs | 2 + crates/ecstore/src/set_disk/metadata.rs | 7 + crates/ecstore/src/set_disk/mod.rs | 2 + crates/ecstore/src/set_disk/ops/object.rs | 318 +++++++++++++++--- crates/ecstore/src/store/init.rs | 8 +- crates/filemeta/src/fileinfo.rs | 55 +++ crates/filemeta/src/filemeta/version.rs | 291 +++++++++++++--- crates/utils/src/http/metadata_compat.rs | 1 + rustfs/src/app/object_usecase.rs | 48 ++- rustfs/src/app/storage_api.rs | 4 + 18 files changed, 1164 insertions(+), 153 deletions(-) diff --git a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs index 9033fe275..5f270668f 100644 --- a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs +++ b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs @@ -554,6 +554,7 @@ async fn delete_free_version_remote_object( oi: &ObjectInfo, tier_config_mgr: &Arc>, ) -> Result<(), std::io::Error> { + let version_id_exact = validate_transition_remote_version(oi)?; let identity = tier_destination_id_from_metadata(&oi.user_defined)? .ok_or_else(|| std::io::Error::other("tier free-version has no durable backend identity"))?; delete_object_from_remote_tier_idempotent_with_manager_and_identity( @@ -562,7 +563,7 @@ async fn delete_free_version_remote_object( &oi.transitioned_object.tier, identity, tier_config_mgr, - false, + version_id_exact, ) .await?; Ok(()) @@ -4201,6 +4202,23 @@ pub async fn get_transitioned_object_reader( get_transitioned_object_reader_with_tier_manager(bucket, object, rs, h, oi, opts, &tier_config_mgr).await } +fn validate_transition_remote_version(oi: &ObjectInfo) -> Result { + let version = oi.transitioned_object.version_id.as_str(); + match oi.transition_version_state { + rustfs_filemeta::TransitionVersionState::Unknown => Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "remote tier object version state is unknown", + )), + rustfs_filemeta::TransitionVersionState::KnownDisabled if version.is_empty() => Ok(false), + rustfs_filemeta::TransitionVersionState::SuspendedNull if version == "null" => Ok(true), + rustfs_filemeta::TransitionVersionState::Exact if !version.is_empty() && version != "null" => Ok(true), + _ => Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "remote tier object version state conflicts with its version ID", + )), + } +} + pub(crate) async fn get_transitioned_object_reader_with_tier_manager( bucket: &str, object: &str, @@ -4210,6 +4228,7 @@ pub(crate) async fn get_transitioned_object_reader_with_tier_manager( opts: &ObjectOptions, tier_config_mgr: &Arc>, ) -> Result { + validate_transition_remote_version(oi)?; let expected_identity = tier_destination_id_from_metadata(&oi.user_defined)?; let lease = match expected_identity { Some(identity) => { @@ -5506,6 +5525,7 @@ mod tests { tier: tier.clone(), ..Default::default() }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Exact, ..Default::default() }; @@ -5569,6 +5589,7 @@ mod tests { tier, ..Default::default() }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Exact, ..Default::default() }; @@ -5591,6 +5612,70 @@ mod tests { assert_eq!(backend.get_count().await, 0); } + #[cfg(feature = "test-util")] + #[tokio::test] + async fn transitioned_get_rejects_unknown_version_state_before_backend_io() { + let manager = TierConfigMgr::new(); + let tier = format!("COLDTIER{}", &Uuid::new_v4().simple().to_string()[..8]).to_uppercase(); + let backend = register_mock_tier(&manager, &tier).await; + let object_info = ObjectInfo { + bucket: "bucket".to_string(), + name: "object".to_string(), + size: 1, + transitioned_object: TransitionedObject { + name: "remote/object".to_string(), + version_id: String::new(), + status: crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE.to_string(), + tier, + ..Default::default() + }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Unknown, + ..Default::default() + }; + + let err = match get_transitioned_object_reader_with_tier_manager( + &object_info.bucket, + &object_info.name, + &None, + &HeaderMap::new(), + &object_info, + &ObjectOptions::default(), + &manager, + ) + .await + { + Ok(_) => panic!("unknown remote version state must fail before backend IO"), + Err(err) => err, + }; + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert_eq!(backend.get_count().await, 0); + } + + #[cfg(feature = "test-util")] + #[tokio::test] + async fn free_version_delete_rejects_unknown_version_state_before_backend_io() { + let manager = TierConfigMgr::new(); + let backend = register_mock_tier(&manager, "WARM").await; + let object_info = ObjectInfo { + transitioned_object: TransitionedObject { + name: "remote/object".to_string(), + version_id: "legacy-version".to_string(), + tier: "WARM".to_string(), + ..Default::default() + }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Unknown, + ..Default::default() + }; + + let err = super::delete_free_version_remote_object(&object_info, &manager) + .await + .expect_err("unknown remote version state must fail before backend IO"); + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert_eq!(backend.remove_count().await, 0); + } + #[cfg(feature = "test-util")] #[tokio::test] async fn free_version_remote_delete_requires_persisted_destination_identity() { @@ -5640,6 +5725,7 @@ mod tests { oi.transitioned_object.tier = "WARM".to_string(); oi.transitioned_object.name = "remote/object".to_string(); oi.transitioned_object.version_id = "remote-version".to_string(); + oi.transition_version_state = rustfs_filemeta::TransitionVersionState::Exact; let local_delete_calls = Arc::new(std::sync::atomic::AtomicUsize::new(0)); let legacy_err = delete_free_version_remote_object_then(&oi, &manager, { @@ -5650,7 +5736,8 @@ mod tests { }) .await .expect_err("legacy free-version without identity must be retained"); - assert!(legacy_err.to_string().contains("no durable backend identity")); + assert_eq!(legacy_err.kind(), std::io::ErrorKind::Other); + assert_eq!(old_backend.remove_count().await, 0); assert_eq!(local_delete_calls.load(Ordering::Relaxed), 0); let mut invalid_metadata = HashMap::new(); @@ -5781,6 +5868,7 @@ mod tests { oi.transitioned_object.tier = "WARM".to_string(); oi.transitioned_object.name = "remote/object".to_string(); oi.transitioned_object.version_id = "remote-version".to_string(); + oi.transition_version_state = rustfs_filemeta::TransitionVersionState::Exact; let err = match get_transitioned_object_reader_with_tier_manager( "bucket", @@ -5796,7 +5884,12 @@ mod tests { Ok(_) => panic!("identity-bound GET must reject a same-name tier rebind"), Err(err) => err, }; - assert!(err.to_string().contains("identity no longer matches")); + assert_eq!(err.kind(), std::io::ErrorKind::Other); + let admin_err = err + .get_ref() + .and_then(|source| source.downcast_ref::()) + .expect("identity mismatch should retain the typed tier error"); + assert_eq!(admin_err.code, crate::services::tier::tier::ERR_TIER_INVALID_CONFIG.code); assert_eq!(new_backend.get_count().await, 0); oi.user_defined = Arc::new(HashMap::new()); @@ -5902,7 +5995,8 @@ mod tests { version_id: "remote-version".to_string(), tier_name: "WARM".to_string(), backend_identity: Some([1; 32]), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; let err = state @@ -6013,7 +6107,8 @@ mod tests { version_id: "remote-version".to_string(), tier_name: "WARM".to_string(), backend_identity: Some([1; 32]), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; state @@ -10081,6 +10176,87 @@ mod tests { (backend, identity_hex) } + #[cfg(feature = "test-util")] + #[tokio::test] + async fn journal_replay_rejects_unknown_version_state_before_backend_io() { + let (_disk_paths, ecstore) = setup_test_env().await; + let (backend, _) = register_recovery_mock_tier(&ecstore).await; + let identity = TierConfigMgr::acquire_operation_lease(&ecstore.tier_config_mgr(), "WARM") + .await + .expect("mock tier lease should be available") + .backend_identity(); + let je = Jentry { + obj_name: "remote/object".to_string(), + version_id: "legacy-version".to_string(), + tier_name: "WARM".to_string(), + backend_identity: Some(identity), + version_id_exact: false, + version_state: rustfs_filemeta::TransitionVersionState::Unknown, + }; + + let err = crate::bucket::lifecycle::tier_delete_journal::process_tier_delete_journal_entry(ecstore, &je) + .await + .expect_err("unknown journal state must fail before backend IO"); + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert_eq!(backend.remove_count().await, 0); + } + + #[cfg(feature = "test-util")] + #[tokio::test] + async fn journal_replay_deletes_confirmed_exact_provider_token() { + let (_disk_paths, ecstore) = setup_test_env().await; + let (backend, _) = register_recovery_mock_tier(&ecstore).await; + let lease = TierConfigMgr::acquire_operation_lease(&ecstore.tier_config_mgr(), "WARM") + .await + .expect("mock tier lease should be available"); + let identity = lease.backend_identity(); + backend + .set_put_remote_version(Some("provider-version-token".to_string())) + .await; + lease + .put( + "remote/object", + crate::client::transition_api::ReaderImpl::Body(bytes::Bytes::from_static(b"candidate")), + 9, + ) + .await + .expect("confirmed remote candidate should be seeded"); + backend.set_remove_failure(true); + backend.set_reject_non_empty_remote_versions(true); + let je = Jentry { + obj_name: "remote/object".to_string(), + version_id: "provider-version-token".to_string(), + tier_name: "WARM".to_string(), + backend_identity: Some(identity), + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, + }; + + crate::set_disk::cleanup_rejected_transition_upload_durably( + &lease, + &je.obj_name, + &je.version_id, + true, + Some(ecstore.clone()), + ) + .await + .expect("failed immediate cleanup should remain durable in the journal"); + assert!(backend.contains(&je.obj_name).await); + + backend.set_remove_failure(false); + crate::bucket::lifecycle::tier_delete_journal::process_tier_delete_journal_entry(ecstore, &je) + .await + .expect("identity-bound exact journal must retry confirmed candidate cleanup"); + + assert!(!backend.contains(&je.obj_name).await); + assert_eq!(backend.exact_remove_count(), 2); + assert_eq!( + backend.remove_versions().await, + vec![("remote/object".to_string(), "provider-version-token".to_string())] + ); + } + async fn seed_recoverable_free_version( disk_paths: &[PathBuf], bucket: &str, @@ -10100,6 +10276,7 @@ mod tests { identity, ); } + let transition_version_id = Uuid::new_v4(); let mut metadata = FileMeta::new(); metadata .add_version(FileInfo { @@ -10108,7 +10285,9 @@ mod tests { version_id: Some(object_version_id), transition_status: crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE.to_string(), transitioned_objname: format!("remote/{bucket}/{object}"), - transition_version_id: Some(Uuid::new_v4()), + transition_version_id: Some(transition_version_id), + transition_version: Some(transition_version_id.to_string()), + transition_version_state: rustfs_filemeta::TransitionVersionState::Exact, transition_tier: "WARM".to_string(), mod_time: Some(OffsetDateTime::now_utc()), metadata: transitioned_metadata, diff --git a/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs b/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs index 39d07ed05..a217d4978 100644 --- a/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs +++ b/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs @@ -20,7 +20,10 @@ use tokio_util::sync::CancellationToken; use tracing::{debug, warn}; use crate::bucket::lifecycle::config_boundary; -use crate::bucket::lifecycle::tier_sweeper::{Jentry, delete_object_from_remote_tier_idempotent_with_manager_and_identity}; +use crate::bucket::lifecycle::tier_sweeper::{ + Jentry, delete_confirmed_transition_candidate_exact_with_manager_and_identity, + delete_object_from_remote_tier_idempotent_with_manager_and_identity, +}; use crate::disk::RUSTFS_META_BUCKET; use crate::error::{Error, Result}; use crate::object_api::{GetObjectReader, ObjectInfo, ObjectOptions, PutObjReader}; @@ -42,6 +45,7 @@ const TIER_DELETE_JOURNAL_RECOVERY_INTERVAL: Duration = Duration::from_secs(60); const TIER_DELETE_JOURNAL_RECOVERY_TIMEOUT: Duration = Duration::from_secs(300); const TIER_DELETE_JOURNAL_VERSION: u8 = 2; const TIER_DELETE_JOURNAL_EXACT_VERSION: u8 = 3; +const TIER_DELETE_JOURNAL_STATE_VERSION: u8 = 4; pub(crate) const TIER_DELETE_JOURNAL_PREFIX: &str = "ilm/tier-delete-journal/"; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -55,24 +59,35 @@ struct PersistedTierDeleteJournalEntry { backend_identity: Option<[u8; 32]>, #[serde(default, skip_serializing_if = "Option::is_none")] version_id_exact: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + version_state: Option, } impl PersistedTierDeleteJournalEntry { - fn from_jentry(je: &Jentry) -> Self { - Self { - version: if je.version_id_exact { - TIER_DELETE_JOURNAL_EXACT_VERSION - } else if je.backend_identity.is_some() { + fn from_jentry(je: &Jentry) -> Result { + validate_version_state(je.version_state, &je.version_id, je.version_id_exact)?; + let legacy_unknown = je.version_state == rustfs_filemeta::TransitionVersionState::Unknown; + let version = if legacy_unknown { + if je.backend_identity.is_some() { TIER_DELETE_JOURNAL_VERSION } else { 1 - }, + } + } else { + if je.backend_identity.is_none() { + return Err(Error::other("new tier delete journal entry is missing its backend identity")); + } + TIER_DELETE_JOURNAL_STATE_VERSION + }; + Ok(Self { + version, obj_name: je.obj_name.clone(), version_id: je.version_id.clone(), tier_name: je.tier_name.clone(), backend_identity: je.backend_identity, version_id_exact: je.version_id_exact.then_some(true), - } + version_state: (!legacy_unknown).then_some(je.version_state), + }) } fn into_jentry(self) -> Result { @@ -84,19 +99,23 @@ impl PersistedTierDeleteJournalEntry { if self.obj_name.is_empty() || self.tier_name.is_empty() { return Err(Error::other("tier delete journal entry is incomplete")); } - if self.version != TIER_DELETE_JOURNAL_EXACT_VERSION && self.version_id_exact.unwrap_or(false) { + if self.version != TIER_DELETE_JOURNAL_EXACT_VERSION + && self.version != TIER_DELETE_JOURNAL_STATE_VERSION + && self.version_id_exact.unwrap_or(false) + { return Err(Error::other( "legacy tier delete journal entry has an unsupported exact version constraint", )); } - let (backend_identity, version_id_exact) = match self.version { - 1 => (None, false), + let (backend_identity, version_id_exact, version_state) = match self.version { + 1 => (None, false, rustfs_filemeta::TransitionVersionState::Unknown), TIER_DELETE_JOURNAL_VERSION => ( Some( self.backend_identity .ok_or_else(|| Error::other("tier delete journal v2 entry is missing its backend identity"))?, ), false, + rustfs_filemeta::TransitionVersionState::Unknown, ), TIER_DELETE_JOURNAL_EXACT_VERSION => { if self.version_id.is_empty() || self.version_id_exact != Some(true) { @@ -108,6 +127,22 @@ impl PersistedTierDeleteJournalEntry { .ok_or_else(|| Error::other("tier delete journal v3 entry is missing its backend identity"))?, ), true, + rustfs_filemeta::TransitionVersionState::Exact, + ) + } + TIER_DELETE_JOURNAL_STATE_VERSION => { + let state = self + .version_state + .ok_or_else(|| Error::other("tier delete journal v4 entry is missing its version state"))?; + let exact = self.version_id_exact.unwrap_or(false); + validate_version_state(state, &self.version_id, exact)?; + ( + Some( + self.backend_identity + .ok_or_else(|| Error::other("tier delete journal v4 entry is missing its backend identity"))?, + ), + exact, + state, ) } version => return Err(Error::other(format!("unsupported tier delete journal version {version}"))), @@ -118,10 +153,30 @@ impl PersistedTierDeleteJournalEntry { tier_name: self.tier_name, backend_identity, version_id_exact, + version_state, }) } } +fn validate_version_state( + state: rustfs_filemeta::TransitionVersionState, + version_id: &str, + version_id_exact: bool, +) -> Result<()> { + use rustfs_filemeta::TransitionVersionState::{Exact, KnownDisabled, SuspendedNull, Unknown}; + + let valid = match state { + Unknown => !version_id_exact, + KnownDisabled => version_id.is_empty() && !version_id_exact, + SuspendedNull => version_id == "null" && version_id_exact, + Exact => !version_id.is_empty() && version_id != "null" && version_id_exact, + }; + if !valid { + return Err(Error::other("tier delete journal version state conflicts with its version id")); + } + Ok(()) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct TierDeleteJournalRecoveryStats { pub scanned: usize, @@ -159,7 +214,7 @@ pub(crate) fn decode_tier_delete_journal_entry(data: &[u8]) -> Result { } pub(crate) fn encode_tier_delete_journal_entry(je: &Jentry) -> Result> { - serde_json::to_vec(&PersistedTierDeleteJournalEntry::from_jentry(je)) + serde_json::to_vec(&PersistedTierDeleteJournalEntry::from_jentry(je)?) .map_err(|err| Error::other(format!("encode tier delete journal failed: {err}"))) } @@ -209,18 +264,35 @@ where } pub async fn process_tier_delete_journal_entry(api: Arc, je: &Jentry) -> std::io::Result<()> { + if je.version_state == rustfs_filemeta::TransitionVersionState::Unknown { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "tier delete journal remote version state is unknown", + )); + } let backend_identity = je .backend_identity .ok_or_else(|| std::io::Error::other("legacy tier delete journal has no durable backend identity"))?; - delete_object_from_remote_tier_idempotent_with_manager_and_identity( - &je.obj_name, - &je.version_id, - &je.tier_name, - backend_identity, - &api.tier_config_mgr(), - je.version_id_exact, - ) - .await?; + if je.version_id_exact { + delete_confirmed_transition_candidate_exact_with_manager_and_identity( + &je.obj_name, + &je.version_id, + &je.tier_name, + backend_identity, + &api.tier_config_mgr(), + ) + .await?; + } else { + delete_object_from_remote_tier_idempotent_with_manager_and_identity( + &je.obj_name, + &je.version_id, + &je.tier_name, + backend_identity, + &api.tier_config_mgr(), + false, + ) + .await?; + } remove_tier_delete_journal_entry(api, je).await } @@ -406,8 +478,9 @@ where #[cfg(test)] mod tests { use super::{ - TIER_DELETE_JOURNAL_EXACT_VERSION, await_tier_delete_journal_recovery, decode_tier_delete_journal_entry, - encode_tier_delete_journal_entry, record_tier_delete_journal_backend_identity, tier_delete_journal_object_name, + TIER_DELETE_JOURNAL_EXACT_VERSION, TIER_DELETE_JOURNAL_STATE_VERSION, await_tier_delete_journal_recovery, + decode_tier_delete_journal_entry, encode_tier_delete_journal_entry, record_tier_delete_journal_backend_identity, + tier_delete_journal_object_name, }; use crate::bucket::lifecycle::tier_sweeper::Jentry; use crate::error::Result; @@ -420,7 +493,8 @@ mod tests { version_id: "remote-version".to_string(), tier_name: "WARM".to_string(), backend_identity: Some([7; 32]), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, } } @@ -436,6 +510,7 @@ mod tests { assert_eq!(decoded.tier_name, je.tier_name); assert_eq!(decoded.backend_identity, je.backend_identity); assert_eq!(decoded.version_id_exact, je.version_id_exact); + assert_eq!(decoded.version_state, je.version_state); } #[test] @@ -450,7 +525,7 @@ mod tests { let persisted: serde_json::Value = serde_json::from_slice(&encoded).expect("exact journal JSON should decode"); let decoded = decode_tier_delete_journal_entry(&encoded).expect("exact journal entry should decode"); - assert_eq!(persisted["version"], TIER_DELETE_JOURNAL_EXACT_VERSION); + assert_eq!(persisted["version"], TIER_DELETE_JOURNAL_STATE_VERSION); assert_eq!(persisted["version_id_exact"], true); assert!(decoded.version_id_exact); assert_ne!(tier_delete_journal_object_name(&exact), tier_delete_journal_object_name(&normalized)); @@ -513,6 +588,46 @@ mod tests { } } + #[test] + fn tier_delete_journal_rejects_conflicting_v4_version_states() { + let identity = vec![7_u8; 32]; + let invalid = [ + ("known-disabled", "unexpected", false), + ("suspended-null", "", true), + ("suspended-null", "null", false), + ("exact", "", true), + ("exact", "null", true), + ("exact", "version", false), + ("unknown", "version", true), + ]; + + for (state, version_id, exact) in invalid { + let persisted = serde_json::json!({ + "version": TIER_DELETE_JOURNAL_STATE_VERSION, + "obj_name": "remote/object", + "version_id": version_id, + "tier_name": "WARM", + "backend_identity": identity, + "version_id_exact": exact.then_some(true), + "version_state": state, + }); + let encoded = serde_json::to_vec(&persisted).expect("invalid journal fixture should encode"); + decode_tier_delete_journal_entry(&encoded).expect_err("conflicting v4 version state must fail closed"); + } + } + + #[test] + fn legacy_journals_decode_with_unknown_version_state() { + let v1 = br#"{"version":1,"obj_name":"remote/object","version_id":"opaque","tier_name":"WARM"}"#; + let v2 = br#"{"version":2,"obj_name":"remote/object","version_id":"opaque","tier_name":"WARM","backend_identity":[7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7]}"#; + + for payload in [v1.as_slice(), v2.as_slice()] { + let decoded = decode_tier_delete_journal_entry(payload).expect("legacy journal should decode"); + assert_eq!(decoded.version_state, rustfs_filemeta::TransitionVersionState::Unknown); + assert!(!decoded.version_id_exact); + } + } + #[test] fn tier_delete_journal_path_is_stable_and_sanitized() { let je = journal_entry(); @@ -530,6 +645,8 @@ mod tests { fn tier_delete_journal_paths_separate_legacy_and_backend_identities() { let mut legacy = journal_entry(); legacy.backend_identity = None; + legacy.version_id_exact = false; + legacy.version_state = rustfs_filemeta::TransitionVersionState::Unknown; let mut backend_a = journal_entry(); backend_a.backend_identity = Some([1; 32]); let mut backend_b = journal_entry(); @@ -575,6 +692,8 @@ mod tests { fn tier_delete_journal_without_transition_identity_stays_legacy() { let mut je = journal_entry(); je.backend_identity = None; + je.version_id_exact = false; + je.version_state = rustfs_filemeta::TransitionVersionState::Unknown; let encoded = encode_tier_delete_journal_entry(&je).expect("legacy journal should remain encodable"); let persisted: serde_json::Value = serde_json::from_slice(&encoded).expect("journal JSON should decode"); diff --git a/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs b/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs index 67ef6d15b..c4ca3b809 100644 --- a/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs +++ b/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs @@ -185,6 +185,7 @@ struct ObjSweeper { transition_status: String, transition_tier: String, transition_version_id: String, + transition_version_state: rustfs_filemeta::TransitionVersionState, remote_object: String, } @@ -231,7 +232,9 @@ impl ObjSweeper { } pub fn should_remove_remote_object(&self) -> Option { - if self.transition_status != lifecycle::TRANSITION_COMPLETE { + if self.transition_status != lifecycle::TRANSITION_COMPLETE + || self.transition_version_state == rustfs_filemeta::TransitionVersionState::Unknown + { return None; } @@ -249,7 +252,11 @@ impl ObjSweeper { version_id: self.transition_version_id.clone(), tier_name: self.transition_tier.clone(), backend_identity: None, - version_id_exact: false, + version_id_exact: matches!( + self.transition_version_state, + rustfs_filemeta::TransitionVersionState::SuspendedNull | rustfs_filemeta::TransitionVersionState::Exact + ), + version_state: self.transition_version_state, }); } None @@ -286,6 +293,7 @@ pub struct Jentry { pub(crate) tier_name: String, pub(crate) backend_identity: Option, pub(crate) version_id_exact: bool, + pub(crate) version_state: rustfs_filemeta::TransitionVersionState, } impl ExpiryOp for Jentry { @@ -330,7 +338,7 @@ async fn delete_object_from_remote_tier_raw_with_manager( let lease = TierConfigMgr::acquire_operation_lease(&tier_config_mgr, tier_name) .await .map_err(std::io::Error::other)?; - delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, &lease, false).await + delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, &lease, false, true).await } async fn delete_object_from_remote_tier_raw_with_lease( @@ -338,8 +346,11 @@ async fn delete_object_from_remote_tier_raw_with_lease( rv_id: &str, lease: &TierOperationLease, version_id_exact: bool, + validate_remote_version_id: bool, ) -> Result<(), std::io::Error> { - lease.validate_remote_version_id(rv_id)?; + if validate_remote_version_id { + lease.validate_remote_version_id(rv_id)?; + } if remote_delete_breaker_is_open(Instant::now()).await { metrics::counter!(METRIC_DELETE_REMOTE_BREAKER_TOTAL).increment(1); @@ -435,7 +446,53 @@ pub(crate) async fn delete_object_from_remote_tier_with_lease_idempotent( lease: &TierOperationLease, version_id_exact: bool, ) -> Result { - match delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, lease, version_id_exact).await { + delete_object_from_remote_tier_with_lease_idempotent_inner(obj_name, rv_id, lease, version_id_exact, true).await +} + +pub(crate) async fn delete_confirmed_transition_candidate_exact_with_lease_idempotent( + obj_name: &str, + rv_id: &str, + lease: &TierOperationLease, +) -> Result { + if rv_id.is_empty() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "confirmed versioned transition candidate requires a non-empty remote version", + )); + } + #[cfg(test)] + if obj_name == "remote/empty-guard-probe" { + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } + delete_object_from_remote_tier_with_lease_idempotent_inner(obj_name, rv_id, lease, true, false).await +} + +#[cfg(test)] +static CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +pub(crate) async fn delete_confirmed_transition_candidate_exact_with_manager_and_identity( + obj_name: &str, + rv_id: &str, + tier_name: &str, + backend_identity: TierDestinationId, + tier_config_mgr: &Arc>, +) -> Result { + let lease = TierConfigMgr::acquire_operation_lease_for_backend_identity(tier_config_mgr, tier_name, backend_identity) + .await + .map_err(std::io::Error::other)?; + delete_confirmed_transition_candidate_exact_with_lease_idempotent(obj_name, rv_id, &lease).await +} + +async fn delete_object_from_remote_tier_with_lease_idempotent_inner( + obj_name: &str, + rv_id: &str, + lease: &TierOperationLease, + version_id_exact: bool, + validate_remote_version_id: bool, +) -> Result { + match delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, lease, version_id_exact, validate_remote_version_id) + .await + { Ok(()) => Ok(RemoteTierDeleteOutcome::Deleted), Err(err) if is_remote_tier_not_found_error(&err) => Ok(RemoteTierDeleteOutcome::AlreadyRemoved), Err(err) => { @@ -460,6 +517,7 @@ pub fn transitioned_delete_journal_entry( versioned: bool, suspended: bool, transitioned: &TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, ) -> Option { let sweeper = ObjSweeper { version_id, @@ -468,6 +526,7 @@ pub fn transitioned_delete_journal_entry( transition_status: transitioned.status.clone(), transition_tier: transitioned.tier.clone(), transition_version_id: transitioned.version_id.clone(), + transition_version_state, remote_object: transitioned.name.clone(), ..Default::default() }; @@ -475,8 +534,13 @@ pub fn transitioned_delete_journal_entry( sweeper.should_remove_remote_object() } -pub fn transitioned_force_delete_journal_entry(transitioned: &TransitionedObject) -> Option { - if transitioned.status != lifecycle::TRANSITION_COMPLETE { +pub fn transitioned_force_delete_journal_entry( + transitioned: &TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, +) -> Option { + if transitioned.status != lifecycle::TRANSITION_COMPLETE + || transition_version_state == rustfs_filemeta::TransitionVersionState::Unknown + { return None; } @@ -485,7 +549,11 @@ pub fn transitioned_force_delete_journal_entry(transitioned: &TransitionedObject version_id: transitioned.version_id.clone(), tier_name: transitioned.tier.clone(), backend_identity: None, - version_id_exact: false, + version_id_exact: matches!( + transition_version_state, + rustfs_filemeta::TransitionVersionState::SuspendedNull | rustfs_filemeta::TransitionVersionState::Exact + ), + version_state: transition_version_state, }) } @@ -494,11 +562,14 @@ mod test { use crate::client::signer_error::invalid_utf8_header_error; use super::{ - ERR_REMOTE_DELETE_BREAKER_OPEN, ERR_REMOTE_DELETE_LIMITER_CLOSED, RemoteDeleteBreaker, RemoteTierDeleteOutcome, + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES, ERR_REMOTE_DELETE_BREAKER_OPEN, ERR_REMOTE_DELETE_LIMITER_CLOSED, + RemoteDeleteBreaker, RemoteTierDeleteOutcome, delete_confirmed_transition_candidate_exact_with_manager_and_identity, delete_object_from_remote_tier_idempotent, delete_object_from_remote_tier_idempotent_with_manager_and_identity, - is_remote_tier_not_found_error, is_signer_header_error, set_remote_tier_delete_test_hook, - should_record_remote_delete_failure, + is_remote_tier_not_found_error, is_signer_header_error, lifecycle, set_remote_tier_delete_test_hook, + should_record_remote_delete_failure, transitioned_delete_journal_entry, transitioned_force_delete_journal_entry, }; + use crate::storage_api_contracts::lifecycle::TransitionedObject; + use rustfs_filemeta::TransitionVersionState; use std::io::{Error, ErrorKind}; use std::time::{Duration, Instant}; @@ -542,6 +613,43 @@ mod test { assert!(should_record_remote_delete_failure(&Error::other("NoSuchVersion"))); } + #[test] + fn transitioned_delete_journal_preserves_remote_version_state() { + let cases = [ + (TransitionVersionState::Unknown, "legacy-version", None), + (TransitionVersionState::KnownDisabled, "", Some(false)), + (TransitionVersionState::SuspendedNull, "null", Some(true)), + (TransitionVersionState::Exact, "opaque-version", Some(true)), + ]; + + for (state, version_id, expected_exact) in cases { + let transitioned = TransitionedObject { + name: "remote/object".to_string(), + version_id: version_id.to_string(), + tier: "WARM".to_string(), + status: lifecycle::TRANSITION_COMPLETE.to_string(), + ..Default::default() + }; + let regular = transitioned_delete_journal_entry(None, false, false, &transitioned, state); + let forced = transitioned_force_delete_journal_entry(&transitioned, state); + + match expected_exact { + Some(expected_exact) => { + let regular = regular.expect("known version state should produce a regular delete journal entry"); + assert_eq!(regular.version_state, state); + assert_eq!(regular.version_id_exact, expected_exact); + let forced = forced.expect("known version state should produce a forced delete journal entry"); + assert_eq!(forced.version_state, state); + assert_eq!(forced.version_id_exact, expected_exact); + } + None => { + assert!(regular.is_none()); + assert!(forced.is_none()); + } + } + } + } + #[tokio::test] #[serial_test::serial] async fn idempotent_remote_delete_treats_hooked_nosuchversion_as_already_removed() { @@ -664,6 +772,55 @@ mod test { assert_eq!(backend.remove_versions().await, vec![("remote/object".to_string(), String::new())]); } + #[cfg(feature = "test-util")] + #[tokio::test] + #[serial_test::serial] + async fn confirmed_transition_cleanup_deletes_exact_provider_token() { + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES.store(0, std::sync::atomic::Ordering::Relaxed); + let manager = crate::services::tier::tier::TierConfigMgr::new(); + let backend = crate::services::tier::test_util::register_mock_tier(&manager, "WARM").await; + let lease = crate::services::tier::tier::TierConfigMgr::acquire_operation_lease(&manager, "WARM") + .await + .expect("test tier lease should be available"); + let identity = lease.backend_identity(); + drop(lease); + backend.set_reject_non_empty_remote_versions(true); + + let outcome = delete_confirmed_transition_candidate_exact_with_manager_and_identity( + "remote/object", + "provider-version-token", + "WARM", + identity, + &manager, + ) + .await + .expect("confirmed upload compensation should delete the exact provider token"); + + assert_eq!(outcome, RemoteTierDeleteOutcome::Deleted); + assert_eq!(backend.exact_remove_count(), 1); + assert_eq!( + backend.remove_versions().await, + vec![("remote/object".to_string(), "provider-version-token".to_string())] + ); + + let err = delete_confirmed_transition_candidate_exact_with_manager_and_identity( + "remote/empty-guard-probe", + "", + "WARM", + identity, + &manager, + ) + .await + .expect_err("confirmed versioned cleanup must reject an empty token"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!(backend.remove_count().await, 1); + assert_eq!( + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES.load(std::sync::atomic::Ordering::Relaxed), + 0, + "empty remote versions must be rejected before exact cleanup dispatch" + ); + } + #[test] fn breaker_opens_at_threshold_and_recovers_after_window() { let mut breaker = RemoteDeleteBreaker::new(3, Duration::from_secs(30)); diff --git a/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs b/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs index 823ae1d2a..e3f3036e8 100644 --- a/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs +++ b/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs @@ -22,7 +22,10 @@ use uuid::Uuid; use crate::bucket::lifecycle::config_boundary; use crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE; -use crate::bucket::lifecycle::tier_sweeper::delete_object_from_remote_tier_idempotent_with_manager_and_identity; +use crate::bucket::lifecycle::tier_sweeper::{ + delete_confirmed_transition_candidate_exact_with_manager_and_identity, + delete_object_from_remote_tier_idempotent_with_manager_and_identity, +}; use crate::disk::RUSTFS_META_BUCKET; use crate::error::{Error, Result as EcstoreResult}; use crate::object_api::ObjectOptions; @@ -708,6 +711,21 @@ async fn recover_unknown_upload_outcome( TransitionCandidateProbe::UnversionedPresent => { cleanup_recovered_unknown_upload_candidate(api, transaction, TransitionRemoteVersion::unversioned()).await } + TransitionCandidateProbe::VersionedPresent(version_id) + if Uuid::parse_str(&version_id).is_ok_and(|version_id| version_id.is_nil()) => + { + delete_confirmed_transition_candidate_exact_with_manager_and_identity( + &transaction.remote_object, + &version_id, + &transaction.tier_name, + transaction.backend_fingerprint, + &api.tier_config_mgr(), + ) + .await + .map_err(Error::other)?; + delete_transition_transaction_record(api, transaction.transaction_id).await?; + Ok(TransitionTransactionRecoveryOutcome::RemoteCandidateDeleted) + } TransitionCandidateProbe::VersionedPresent(version_id) => { cleanup_recovered_unknown_upload_candidate(api, transaction, TransitionRemoteVersion::versioned(version_id)).await } diff --git a/crates/ecstore/src/config/com.rs b/crates/ecstore/src/config/com.rs index 0f3eb9ab5..539464173 100644 --- a/crates/ecstore/src/config/com.rs +++ b/crates/ecstore/src/config/com.rs @@ -2543,6 +2543,7 @@ mod tests { data_dir: None, delete_marker: false, transitioned_object: Default::default(), + transition_version_state: Default::default(), restore_ongoing: false, restore_expires: None, user_tags: Arc::new(String::new()), diff --git a/crates/ecstore/src/object_api/types.rs b/crates/ecstore/src/object_api/types.rs index c9a0e560b..dfd403e18 100644 --- a/crates/ecstore/src/object_api/types.rs +++ b/crates/ecstore/src/object_api/types.rs @@ -180,6 +180,7 @@ pub struct ObjectInfo { pub data_dir: Option, pub delete_marker: bool, pub transitioned_object: TransitionedObject, + pub transition_version_state: rustfs_filemeta::TransitionVersionState, pub restore_ongoing: bool, pub restore_expires: Option, pub user_tags: Arc, @@ -220,6 +221,7 @@ impl Clone for ObjectInfo { data_dir: self.data_dir, delete_marker: self.delete_marker, transitioned_object: self.transitioned_object.clone(), + transition_version_state: self.transition_version_state, restore_ongoing: self.restore_ongoing, restore_expires: self.restore_expires, user_tags: self.user_tags.clone(), @@ -464,11 +466,11 @@ impl ObjectInfo { let transitioned_object = TransitionedObject { name: fi.transitioned_objname.clone(), - version_id: if let Some(transition_version_id) = fi.transition_version_id { - transition_version_id.to_string() - } else { - "".to_string() - }, + version_id: fi + .transition_version + .clone() + .or_else(|| fi.transition_version_id.map(|version_id| version_id.to_string())) + .unwrap_or_default(), status: fi.transition_status.clone(), free_version: fi.tier_free_version(), tier: fi.transition_tier.clone(), @@ -537,6 +539,7 @@ impl ObjectInfo { inlined, user_defined: Arc::new(metadata), transitioned_object, + transition_version_state: fi.transition_version_state, checksum: fi.checksum.clone(), storage_class, restore_ongoing, diff --git a/crates/ecstore/src/services/tier/test_util.rs b/crates/ecstore/src/services/tier/test_util.rs index 1a2b37d26..751f2e92b 100644 --- a/crates/ecstore/src/services/tier/test_util.rs +++ b/crates/ecstore/src/services/tier/test_util.rs @@ -931,7 +931,10 @@ pub async fn read_transition_meta(disk_path: &Path, bucket: &str, object: &str) status: fi.transition_status.clone(), tier: fi.transition_tier.clone(), remote_object: fi.transitioned_objname.clone(), - remote_version_id: fi.transition_version_id.map(|id| id.to_string()), + remote_version_id: fi + .transition_version + .clone() + .or_else(|| fi.transition_version_id.map(|id| id.to_string())), free_version_count, }) } diff --git a/crates/ecstore/src/services/tier/tier.rs b/crates/ecstore/src/services/tier/tier.rs index c501d52ec..5ead6a9d9 100644 --- a/crates/ecstore/src/services/tier/tier.rs +++ b/crates/ecstore/src/services/tier/tier.rs @@ -9274,7 +9274,8 @@ mod tests { version_id: "v1".to_string(), tier_name: "COLD-A".to_string(), backend_identity: Some(current_identity), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; journal_store .insert_config_object( diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index 4972274f1..b8fd83ced 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -547,6 +547,8 @@ pub(in crate::set_disk) fn metadata_early_stop_candidate_matches(left: &FileInfo && left.transitioned_objname == right.transitioned_objname && left.transition_tier == right.transition_tier && left.transition_version_id == right.transition_version_id + && left.transition_version == right.transition_version + && left.transition_version_state == right.transition_version_state && left.expire_restored == right.expire_restored && left.size == right.size && left.mod_time == right.mod_time diff --git a/crates/ecstore/src/set_disk/metadata.rs b/crates/ecstore/src/set_disk/metadata.rs index 136a6cf7e..7eb4c57ad 100644 --- a/crates/ecstore/src/set_disk/metadata.rs +++ b/crates/ecstore/src/set_disk/metadata.rs @@ -578,6 +578,13 @@ impl SetDisks { Self::update_hash_str(hasher, &meta.transition_tier); Self::update_hash_str(hasher, &meta.transitioned_objname); Self::update_hash_optional_uuid(hasher, meta.transition_version_id); + Self::update_hash_optional_str(hasher, meta.transition_version.as_deref()); + hasher.update([match meta.transition_version_state { + rustfs_filemeta::TransitionVersionState::Unknown => 0, + rustfs_filemeta::TransitionVersionState::KnownDisabled => 1, + rustfs_filemeta::TransitionVersionState::SuspendedNull => 2, + rustfs_filemeta::TransitionVersionState::Exact => 3, + }]); Self::update_hash_optional_u32(hasher, meta.mode); Self::update_hash_optional_u64(hasher, meta.written_by_version); diff --git a/crates/ecstore/src/set_disk/mod.rs b/crates/ecstore/src/set_disk/mod.rs index 692571243..16accb2dd 100644 --- a/crates/ecstore/src/set_disk/mod.rs +++ b/crates/ecstore/src/set_disk/mod.rs @@ -692,6 +692,8 @@ pub(crate) use ops::multipart::{MultipartCommitBarrier, MultipartCommitPause}; #[cfg(feature = "test-util")] pub(crate) use ops::object::TransitionCleanupStoreBarrier as SetDiskTransitionCleanupStoreBarrier; pub(crate) use ops::object::body_cache_plaintext_len; +#[cfg(test)] +pub(crate) use ops::object::cleanup_rejected_transition_upload_durably; mod read; mod replication; pub(crate) mod shard_source; diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 7725c2292..41775da3e 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -25,7 +25,10 @@ use crate::set_disk::read::GetObjectDownstreamWriter; use crate::bucket::lifecycle::{ tier_delete_journal::{persist_tier_delete_journal_entry, remove_tier_delete_journal_entry}, - tier_sweeper::{Jentry, RemoteTierDeleteOutcome, delete_object_from_remote_tier_with_lease_idempotent}, + tier_sweeper::{ + Jentry, RemoteTierDeleteOutcome, delete_confirmed_transition_candidate_exact_with_lease_idempotent, + delete_object_from_remote_tier_with_lease_idempotent, + }, transition_transaction::{ TransitionRemoteVersion, TransitionSourceIdentity, TransitionSourceVersionMode, TransitionTransaction, TransitionTransactionInit, TransitionTransactionState, delete_transition_transaction_record, @@ -1663,7 +1666,11 @@ pub(crate) async fn cleanup_uncommitted_transition_upload( cleanup_version: &str, version_id_exact: bool, ) -> std::io::Result { - delete_object_from_remote_tier_with_lease_idempotent(object, cleanup_version, lease, version_id_exact).await + if version_id_exact { + delete_confirmed_transition_candidate_exact_with_lease_idempotent(object, cleanup_version, lease).await + } else { + delete_object_from_remote_tier_with_lease_idempotent(object, cleanup_version, lease, false).await + } } fn log_transition_upload_cleanup_failure(lease: &TierOperationLease, object: &str, cleanup_version: &str, err: &std::io::Error) { @@ -1800,7 +1807,7 @@ impl Drop for TransitionUploadCleanup { } } -async fn cleanup_rejected_transition_upload_durably( +pub(crate) async fn cleanup_rejected_transition_upload_durably( lease: &TierOperationLease, object: &str, cleanup_version: &str, @@ -1813,6 +1820,13 @@ async fn cleanup_rejected_transition_upload_durably( tier_name: lease.tier_name().to_string(), backend_identity: Some(lease.backend_identity()), version_id_exact, + version_state: if !version_id_exact { + rustfs_filemeta::TransitionVersionState::KnownDisabled + } else if cleanup_version == "null" { + rustfs_filemeta::TransitionVersionState::SuspendedNull + } else { + rustfs_filemeta::TransitionVersionState::Exact + }, }; let journal_error = if let Some(api) = api.as_ref() { @@ -1972,12 +1986,83 @@ async fn advance_and_save_transition_transaction( next: TransitionTransactionState, remote_version: Option, ) -> Result<()> { + #[cfg(test)] + record_transition_uploaded_save_attempt(transaction, next); transaction .advance(transaction.fence(), next, remote_version) .map_err(Error::other)?; save_transition_transaction_if_available(api, transaction).await } +#[cfg(test)] +struct TransitionUploadedSaveProbeState { + bucket: String, + object: String, + attempts: std::sync::atomic::AtomicUsize, +} + +#[cfg(test)] +struct TransitionUploadedSaveProbe { + state: Arc, +} + +#[cfg(test)] +static TRANSITION_UPLOADED_SAVE_PROBE: std::sync::OnceLock>>> = + std::sync::OnceLock::new(); + +#[cfg(test)] +impl TransitionUploadedSaveProbe { + fn install(bucket: &str, object: &str) -> Self { + let state = Arc::new(TransitionUploadedSaveProbeState { + bucket: bucket.to_string(), + object: object.to_string(), + attempts: std::sync::atomic::AtomicUsize::new(0), + }); + let mut slot = TRANSITION_UPLOADED_SAVE_PROBE + .get_or_init(|| std::sync::Mutex::new(None)) + .lock() + .expect("transition uploaded-save probe mutex should not poison"); + assert!(slot.is_none(), "transition uploaded-save probe must be installed by one test at a time"); + *slot = Some(Arc::clone(&state)); + drop(slot); + Self { state } + } + + fn attempts(&self) -> usize { + self.state.attempts.load(std::sync::atomic::Ordering::Acquire) + } +} + +#[cfg(test)] +impl Drop for TransitionUploadedSaveProbe { + fn drop(&mut self) { + let mut slot = TRANSITION_UPLOADED_SAVE_PROBE + .get_or_init(|| std::sync::Mutex::new(None)) + .lock() + .expect("transition uploaded-save probe mutex should not poison"); + if slot.as_ref().is_some_and(|state| Arc::ptr_eq(state, &self.state)) { + *slot = None; + } + } +} + +#[cfg(test)] +fn record_transition_uploaded_save_attempt(transaction: &TransitionTransaction, next: TransitionTransactionState) { + if next != TransitionTransactionState::Uploaded { + return; + } + let state = TRANSITION_UPLOADED_SAVE_PROBE + .get_or_init(|| std::sync::Mutex::new(None)) + .lock() + .expect("transition uploaded-save probe mutex should not poison") + .as_ref() + .filter(|state| state.bucket == transaction.source.bucket && state.object == transaction.source.object) + .cloned(); + if let Some(state) = state { + state.attempts.fetch_add(1, std::sync::atomic::Ordering::AcqRel); + } +} + async fn delete_transition_transaction_if_available(api: Option<&Arc>, transaction_id: Uuid) -> Result<()> { if let Some(api) = api { return delete_transition_transaction_record(api.clone(), transaction_id).await; @@ -2228,11 +2313,25 @@ async fn pause_transition_commit(bucket: &str, object: &str, pause: TransitionCo } } -fn parse_transition_version_id(remote_version: &str) -> std::result::Result, uuid::Error> { +fn persisted_transition_version( + remote_version: &str, +) -> std::io::Result<(Option, rustfs_filemeta::TransitionVersionState)> { if remote_version.is_empty() { - return Ok(None); + return Ok((None, rustfs_filemeta::TransitionVersionState::KnownDisabled)); } - Uuid::parse_str(remote_version).map(|version_id| (!version_id.is_nil()).then_some(version_id)) + let version_id = Uuid::parse_str(remote_version).map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::Unsupported, + "opaque remote tier versions require the cluster capability gate", + ) + })?; + if version_id.is_nil() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "remote tier returned a nil object version ID", + )); + } + Ok((Some(remote_version.to_string()), rustfs_filemeta::TransitionVersionState::Exact)) } #[cfg(test)] @@ -2470,16 +2569,17 @@ mod transition_upload_completion_tests { #[cfg(test)] mod transition_version_id_tests { - use super::{TransitionUploadCandidate, parse_transition_version_id}; + use super::{TransitionUploadCandidate, persisted_transition_version}; + use rustfs_filemeta::TransitionVersionState; use uuid::Uuid; #[test] fn normalizes_persisted_unversioned_ids_and_preserves_put_constraints() { - assert_eq!(parse_transition_version_id("").expect("empty remote version should be valid"), None); assert_eq!( - parse_transition_version_id(&Uuid::nil().to_string()).expect("nil remote version should be valid"), - None + persisted_transition_version("").expect("empty remote version identifies an unversioned tier"), + (None, TransitionVersionState::KnownDisabled) ); + assert!(persisted_transition_version(&Uuid::nil().to_string()).is_err()); let nil_put_response = Uuid::nil().to_string(); let nil_candidate = TransitionUploadCandidate::from_put_response(nil_put_response.clone()); assert_eq!(nil_candidate.cleanup_version(), nil_put_response); @@ -2491,12 +2591,14 @@ mod transition_version_id_tests { } #[test] - fn preserves_valid_remote_id_and_rejects_invalid_text() { + fn preserves_uuid_and_gates_opaque_remote_ids() { let version_id = Uuid::new_v4(); assert_eq!( - parse_transition_version_id(&version_id.to_string()).expect("UUID remote version should be valid"), - Some(version_id) + persisted_transition_version(&version_id.to_string()).expect("UUID remote version"), + (Some(version_id.to_string()), TransitionVersionState::Exact) ); + assert!(persisted_transition_version("null").is_err()); + assert!(persisted_transition_version("opaque-version-token").is_err()); assert_eq!( TransitionUploadCandidate::from_put_response(version_id.to_string()).cleanup_version(), version_id.to_string() @@ -2505,7 +2607,6 @@ mod transition_version_id_tests { TransitionUploadCandidate::from_put_response("opaque-version-token".to_string()).cleanup_version(), "opaque-version-token" ); - assert!(parse_transition_version_id("not-a-uuid").is_err()); } } @@ -3775,6 +3876,20 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object).await; return Err(err.into()); } + let (transition_version_id, transition_version_state) = match persisted_transition_version(candidate.remote_version()) { + Ok(version) => version, + Err(err) => { + let cleanup_api = transition_cleanup_store(&self.ctx).await; + if let Err(cleanup_err) = upload_cleanup.cleanup_rejected_upload(cleanup_api).await { + return Err(StorageError::Io(std::io::Error::other(format!( + "{err}; rejected remote upload cleanup failed: {cleanup_err}" + )))); + } + delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object) + .await; + return Err(err.into()); + } + }; if let Err(err) = advance_and_save_transition_transaction( transaction_api.as_ref(), &mut transaction, @@ -3792,16 +3907,6 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object).await; return Err(err); } - let transition_version_id = match parse_transition_version_id(candidate.remote_version()) { - Ok(version_id) => version_id, - Err(err) => { - if upload_cleanup.cleanup().await.is_ok() { - delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object) - .await; - } - return Err(err.into()); - } - }; let mut commit_opts = opts.clone(); commit_opts.no_lock = true; @@ -3859,7 +3964,11 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { current_fi.transition_status = TRANSITION_COMPLETE.to_string(); current_fi.transitioned_objname = dest_obj; current_fi.transition_tier = opts.transition.tier.clone(); - current_fi.transition_version_id = transition_version_id; + current_fi.transition_version_id = transition_version_id + .as_deref() + .and_then(|version_id| Uuid::parse_str(version_id).ok()); + current_fi.transition_version = transition_version_id; + current_fi.transition_version_state = transition_version_state; rustfs_utils::http::metadata_compat::insert_str( &mut current_fi.metadata, rustfs_utils::http::metadata_compat::SUFFIX_TRANSITION_TIER_DESTINATION_ID, @@ -4668,6 +4777,33 @@ mod transition_commit_failure_tests { } #[tokio::test] + async fn rejected_unsupported_remote_versions_are_cleaned_up() { + for remote_version in ["null", "opaque-version-token"] { + let manager = TierConfigMgr::new(); + let backend = register_mock_tier(&manager, "WARM").await; + let lease = TierConfigMgr::acquire_operation_lease(&manager, "WARM") + .await + .expect("mock tier lease should be available"); + let candidate = TransitionUploadCandidate::from_put_response(remote_version.to_string()); + + persisted_transition_version(candidate.remote_version()).expect_err("unsupported writer version must fail closed"); + cleanup_rejected_transition_upload_durably( + &lease, + "remote/object", + candidate.cleanup_version(), + candidate.cleanup_version_is_exact(), + None, + ) + .await + .expect("rejected remote upload must be cleaned up"); + + assert_eq!( + backend.remove_versions().await, + vec![("remote/object".to_string(), candidate.cleanup_version().to_string())] + ); + } + } + #[serial_test::serial(restore_multipart_failure_point)] async fn multipart_restore_aborts_every_post_create_failure() { let (temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; @@ -6615,6 +6751,45 @@ mod transition_upload_integrity_tests { ); } + #[tokio::test] + #[serial_test::serial] + async fn unversioned_remote_version_is_persisted_without_version_id() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "transition-unversioned-tier-bucket"; + let object = "object.bin"; + let payload = b"unversioned remote tier must commit without a version id".repeat(1024); + let original = write_source(&set_disks, &disk_stores, bucket, object, &payload).await; + let tier_name = format!("COLDTIER{}", &Uuid::new_v4().simple().to_string()[..8]).to_uppercase(); + let backend = register_mock_tier(&runtime_sources::global_tier_config_mgr(), &tier_name).await; + backend.set_put_remote_version(Some(String::new())).await; + let save_probe = TransitionUploadedSaveProbe::install(bucket, object); + + set_disks + .transition_object(bucket, object, &transition_options(&original, tier_name)) + .await + .expect("an unversioned remote version must commit"); + let (fi, _, _) = set_disks + .get_object_fileinfo( + bucket, + object, + &ObjectOptions { + no_lock: true, + metadata_cache_safe: false, + ..Default::default() + }, + true, + false, + ) + .await + .expect("committed unversioned transition metadata should be readable"); + assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version, None); + assert_eq!(fi.transition_version_state, rustfs_filemeta::TransitionVersionState::KnownDisabled); + assert_eq!(save_probe.attempts(), 1); + assert_eq!(backend.remove_count().await, 0); + assert_eq!(backend.object_count().await, 1); + } + #[tokio::test] #[serial_test::serial] async fn opaque_remote_version_is_cleaned_before_parse_failure() { @@ -6630,7 +6805,7 @@ mod transition_upload_integrity_tests { set_disks .transition_object(bucket, object, &transition_options(&original, tier_name)) .await - .expect_err("an unparseable remote version must fail closed"); + .expect_err("an opaque remote version must fail closed until the capability gate is active"); let removed_versions = backend.remove_versions().await; assert_eq!(removed_versions.len(), 1); assert_eq!(removed_versions[0].1, "opaque-version-token"); @@ -6638,6 +6813,38 @@ mod transition_upload_integrity_tests { assert_local_source_intact(&set_disks, bucket, object, &payload).await; } + #[tokio::test] + #[serial_test::serial] + async fn nil_remote_version_is_cleaned_exactly_before_transaction_persistence() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "transition-nil-version-bucket"; + let object = "object.bin"; + let payload = b"nil remote version must retain local data".repeat(1024); + let original = write_source(&set_disks, &disk_stores, bucket, object, &payload).await; + let tier_name = format!("COLDTIER{}", &Uuid::new_v4().simple().to_string()[..8]).to_uppercase(); + let remote_version = Uuid::nil().to_string(); + let backend = register_mock_tier(&runtime_sources::global_tier_config_mgr(), &tier_name).await; + backend.set_put_remote_version(Some(remote_version.clone())).await; + let save_probe = TransitionUploadedSaveProbe::install(bucket, object); + + set_disks + .transition_object(bucket, object, &transition_options(&original, tier_name)) + .await + .expect_err("a nil remote version must fail closed before transaction persistence"); + let put_versions = backend.put_versions().await; + let removed_versions = backend.remove_versions().await; + assert_eq!(removed_versions, put_versions); + assert_eq!(removed_versions.len(), 1); + assert_eq!( + removed_versions.first().map(|(_, version)| version.as_str()), + Some(remote_version.as_str()) + ); + assert_eq!(save_probe.attempts(), 0, "nil remote version must be rejected before saving Uploaded"); + assert_eq!(backend.exact_remove_count(), 1); + assert_eq!(backend.object_count().await, 0); + assert_local_source_intact(&set_disks, bucket, object, &payload).await; + } + #[tokio::test] #[serial_test::serial] async fn authoritative_read_failure_after_upload_cleans_exact_candidate_and_preserves_source() { @@ -6985,23 +7192,36 @@ mod transition_source_identity_matrix_tests { let object = format!("identity-{index}.bin"); let payload = vec![u8::try_from(index + 1).expect("matrix index should fit u8"); 1024 * 1024]; let mut reader = PutObjReader::from_vec(payload); + let source_version_id = Uuid::new_v4(); + let source_opts = ObjectOptions { + version_id: Some(source_version_id.to_string()), + versioned: true, + ..Default::default() + }; let original = set_disks - .put_object(bucket, &object, &mut reader, &ObjectOptions::default()) + .put_object(bucket, &object, &mut reader, &source_opts) .await .expect("source object should be written"); let (source, _, _) = set_disks - .get_object_fileinfo(bucket, &object, &ObjectOptions::default(), true, false) + .get_object_fileinfo(bucket, &object, &source_opts, true, false) .await .expect("source metadata should resolve"); + assert_eq!(source.version_id, Some(source_version_id)); + assert_eq!( + transition_source_identity(bucket, &object, &source, &source_opts, &get_raw_etag(&source.metadata)) + .expect("persisted versioned source identity should build") + .version_mode, + TransitionSourceVersionMode::Versioned + ); let opts = ObjectOptions { no_lock: true, + versioned: true, transition: TransitionOptions { status: TRANSITION_PENDING.to_string(), tier: tier_name.clone(), etag: original.etag.clone().unwrap_or_default(), ..Default::default() }, - version_id: original.version_id.map(|version| version.to_string()), mod_time: original.mod_time, ..Default::default() }; @@ -7014,7 +7234,10 @@ mod transition_source_identity_matrix_tests { let mut changed = source.clone(); match field { - IdentityField::VersionId => changed.version_id = Some(Uuid::new_v4()), + IdentityField::VersionId => { + changed.version_id = Some(Uuid::new_v4()); + changed.fresh = true; + } IdentityField::DataDir => changed.data_dir = Some(Uuid::new_v4()), IdentityField::ModTime => { changed.mod_time = changed.mod_time.map(|value| value + time::Duration::nanoseconds(1)); @@ -7032,12 +7255,19 @@ mod transition_source_identity_matrix_tests { .await .expect("single-field metadata drift should be written"); } + let persisted_opts = ObjectOptions { + version_id: changed.version_id.map(|version_id| version_id.to_string()), + versioned: true, + ..Default::default() + }; + let (persisted, _, _) = set_disks + .get_object_fileinfo(bucket, &object, &persisted_opts, true, false) + .await + .expect("drifted source metadata should resolve"); put_barrier.release(); - transition - .await - .expect("transition task should not panic") - .expect_err("transition must reject a source whose identity changed after upload"); + let result = transition.await.expect("transition task should not panic"); + assert!(result.is_err(), "transition must reject {field:?} drift"); let expected_attempts = index + 1; assert_eq!(backend.put_count().await, expected_attempts); assert_eq!(backend.remove_count().await, expected_attempts); @@ -7048,26 +7278,26 @@ mod transition_source_identity_matrix_tests { ); match field { - IdentityField::VersionId => assert_ne!(source.version_id, changed.version_id), - IdentityField::DataDir => assert_ne!(source.data_dir, changed.data_dir), - IdentityField::ModTime => assert_ne!(source.mod_time, changed.mod_time), - IdentityField::Size => assert_ne!(source.size, changed.size), - IdentityField::Etag => assert_ne!(get_raw_etag(&source.metadata), get_raw_etag(&changed.metadata)), + IdentityField::VersionId => assert_ne!(source.version_id, persisted.version_id), + IdentityField::DataDir => assert_ne!(source.data_dir, persisted.data_dir), + IdentityField::ModTime => assert_ne!(source.mod_time, persisted.mod_time), + IdentityField::Size => assert_ne!(source.size, persisted.size), + IdentityField::Etag => assert_ne!(get_raw_etag(&source.metadata), get_raw_etag(&persisted.metadata)), } if !matches!(field, IdentityField::VersionId) { - assert_eq!(source.version_id, changed.version_id); + assert_eq!(source.version_id, persisted.version_id); } if !matches!(field, IdentityField::DataDir) { - assert_eq!(source.data_dir, changed.data_dir); + assert_eq!(source.data_dir, persisted.data_dir); } if !matches!(field, IdentityField::ModTime) { - assert_eq!(source.mod_time, changed.mod_time); + assert_eq!(source.mod_time, persisted.mod_time); } if !matches!(field, IdentityField::Size) { - assert_eq!(source.size, changed.size); + assert_eq!(source.size, persisted.size); } if !matches!(field, IdentityField::Etag) { - assert_eq!(get_raw_etag(&source.metadata), get_raw_etag(&changed.metadata)); + assert_eq!(get_raw_etag(&source.metadata), get_raw_etag(&persisted.metadata)); } } } diff --git a/crates/ecstore/src/store/init.rs b/crates/ecstore/src/store/init.rs index 5c410839e..1ce3bbd7f 100644 --- a/crates/ecstore/src/store/init.rs +++ b/crates/ecstore/src/store/init.rs @@ -1311,14 +1311,16 @@ mod tests { version_id: "version-a".to_string(), tier_name: tier_a.to_string(), backend_identity: Some(identity_a), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; let entry_b = Jentry { obj_name: "remote-b".to_string(), version_id: "version-b".to_string(), tier_name: tier_b.to_string(), backend_identity: Some(identity_b), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; let remove_a = backend_a.arm_failing_remove_barrier().await; persist_tier_delete_journal_entry(store_a.clone(), &entry_a) @@ -2720,10 +2722,12 @@ mod tests { #[serial_test::serial(storage_class_env)] async fn transition_transaction_recovery_deletes_provider_recovered_unknown_upload() { let versioned_remote = uuid::Uuid::new_v4().to_string(); + let nil_remote = uuid::Uuid::nil().to_string(); for (case, tier_name, remote_version) in [ ("missing", "TXPROBEMISSING", None), ("unversioned", "TXPROBEUNVERSIONED", Some(String::new())), ("versioned", "TXPROBEVERSIONED", Some(versioned_remote)), + ("nil-version", "TXPROBENILVERSION", Some(nil_remote)), ] { let temp_dir = tempfile::tempdir().expect("create temp store dir"); let (ctx, store, _shutdown) = without_storage_class_env(build_isolated_test_store( diff --git a/crates/filemeta/src/fileinfo.rs b/crates/filemeta/src/fileinfo.rs index 2f1714e01..e1f1f2e1b 100644 --- a/crates/filemeta/src/fileinfo.rs +++ b/crates/filemeta/src/fileinfo.rs @@ -219,6 +219,16 @@ impl ErasureInfo { } // #[derive(Debug, Clone)] +#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy, Default)] +#[serde(rename_all = "kebab-case")] +pub enum TransitionVersionState { + #[default] + Unknown, + KnownDisabled, + SuspendedNull, + Exact, +} + #[derive(Serialize, Deserialize, Debug, PartialEq, Clone, Default)] pub struct FileInfo { pub volume: String, @@ -230,6 +240,10 @@ pub struct FileInfo { pub transitioned_objname: String, pub transition_tier: String, pub transition_version_id: Option, + #[serde(default)] + pub transition_version: Option, + #[serde(default)] + pub transition_version_state: TransitionVersionState, pub expire_restored: bool, pub data_dir: Option, pub mod_time: Option, @@ -459,6 +473,10 @@ impl FileInfo { if self.mod_time.is_none_or(|mod_time| mod_time <= OffsetDateTime::UNIX_EPOCH) || (!allow_nil_version_id && self.version_id.is_some_and(|version_id| version_id.is_nil())) || self.transition_version_id.is_some_and(|version_id| version_id.is_nil()) + || self + .transition_version + .as_ref() + .is_some_and(|version_id| version_id.is_empty()) || self.size != 0 || self.data_dir.is_some() || self.mode.is_some() @@ -492,6 +510,7 @@ impl FileInfo { || !self.transitioned_objname.is_empty() || !self.transition_tier.is_empty() || self.transition_version_id.is_some() + || self.transition_version.is_some() || self.expire_restored || self.size != 0 || self.data_dir.is_some() @@ -536,6 +555,25 @@ impl FileInfo { /// return `None`. pub fn validate(&self, mode: ValidationMode) -> Result> { self.validate_collection_bounds()?; + if let (Some(version), Some(version_id)) = (&self.transition_version, self.transition_version_id) + && Uuid::parse_str(version).ok() != Some(version_id) + { + return Err(Error::FileCorrupt); + } + let transition_state_valid = match self.transition_version_state { + TransitionVersionState::Unknown => true, + TransitionVersionState::KnownDisabled => self.transition_version.is_none() && self.transition_version_id.is_none(), + TransitionVersionState::SuspendedNull => { + self.transition_version.as_deref() == Some("null") && self.transition_version_id.is_none() + } + TransitionVersionState::Exact => self + .transition_version + .as_deref() + .is_some_and(|version| version != "null" && !version.is_empty()), + }; + if !transition_state_valid { + return Err(Error::FileCorrupt); + } let erasure_layout = match mode { ValidationMode::RequireErasure => Some(self.validate_erasure_geometry()?), @@ -832,6 +870,8 @@ impl FileInfo { && self.transition_tier == other.transition_tier && self.transitioned_objname == other.transitioned_objname && self.transition_version_id == other.transition_version_id + && self.transition_version == other.transition_version + && self.transition_version_state == other.transition_version_state } /// Check if metadata maps are equal @@ -1351,6 +1391,15 @@ mod tests { assert_file_corrupt(&fi, ValidationMode::DeleteOnly); } + #[test] + fn metadata_read_validation_rejects_conflicting_transition_versions() { + let mut fi = one_shard_validation_fileinfo(1); + fi.transition_version_id = Some(Uuid::new_v4()); + fi.transition_version = Some(Uuid::new_v4().to_string()); + + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + } + #[test] fn metadata_read_validation_requires_canonical_delete_marker_shape() { let marker = FileInfo { @@ -1722,6 +1771,12 @@ mod tests { transitioned_objname, transition_tier, transition_version_id, + transition_version: transition_version_id.map(|version_id| version_id.to_string()), + transition_version_state: if transition_version_id.is_some() { + TransitionVersionState::Exact + } else { + TransitionVersionState::Unknown + }, expire_restored, data_dir, mod_time, diff --git a/crates/filemeta/src/filemeta/version.rs b/crates/filemeta/src/filemeta/version.rs index f491e75b9..091bfb527 100644 --- a/crates/filemeta/src/filemeta/version.rs +++ b/crates/filemeta/src/filemeta/version.rs @@ -26,13 +26,14 @@ use super::msgp_decode::{ PrependByteReader, prealloc_hint, read_exact_vec, read_nil_or_array_len, read_nil_or_map_len, skip_msgp_value, }; use super::*; -use crate::ChecksumInfo; +use crate::{ChecksumInfo, TransitionVersionState}; use rustfs_utils::HashAlgorithm; use rustfs_utils::http::{ RUSTFS_INTERNAL_PREFIX, SUFFIX_CRC, SUFFIX_FREE_VERSION, SUFFIX_INLINE_DATA, SUFFIX_PURGESTATUS, SUFFIX_TIER_FV_ID, SUFFIX_TIER_FV_MARKER, SUFFIX_TRANSITION_STATUS, SUFFIX_TRANSITION_TIER, SUFFIX_TRANSITION_TIER_DESTINATION_ID, - SUFFIX_TRANSITIONED_OBJECTNAME, SUFFIX_TRANSITIONED_VERSION_ID, contains_key_bytes, get_bytes, get_consistent_bytes, get_str, - has_internal_suffix, insert_bytes, is_internal_key, remove_bytes, strip_internal_prefix, + SUFFIX_TRANSITIONED_OBJECTNAME, SUFFIX_TRANSITIONED_VERSION_ID, SUFFIX_TRANSITIONED_VERSION_STATE, contains_key_bytes, + get_bytes, get_consistent_bytes, get_str, has_internal_suffix, insert_bytes, is_internal_key, remove_bytes, + strip_internal_prefix, }; const MSGPACK_EXT8: u8 = 0xc7; @@ -43,6 +44,7 @@ const MSGPACK_FIXEXT8: u8 = 0xd7; const MSGPACK_TIME_EXT_LEGACY: i8 = 5; const MSGPACK_TIME_EXT_OFFICIAL: i8 = -1; const MSGPACK_TIME_LEN: u8 = 12; +const MAX_TRANSITION_VERSION_LEN: usize = 1024; /// Sentinel signature returned when a version has no computable body (invalid / /// missing inner object). Mirrors MinIO's `signatureErr` so such versions never @@ -251,23 +253,93 @@ fn parse_legacy_uuid_bytes(bytes: &[u8], field: &str) -> Result> { /// Decode a stored transitioned-version-id from a version's `meta_sys`. /// -/// RustFS writes it as 16 raw UUID bytes; MinIO-migrated tiered objects store -/// the remote tier's version id as a UUID *string*. Accept both, and treat any -/// absent / nil / otherwise-unparseable value as "no tier version" (matching the -/// tolerant pre-hardening behavior) rather than failing the whole object read — -/// a malformed tier id must not make an otherwise-readable object unreadable. -fn transitioned_version_id_from_meta_sys(meta_sys: &HashMap>) -> Option { - let value = get_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_ID)?; +/// Legacy RustFS writes used 16 raw UUID bytes. New writes and MinIO-migrated +/// records use the provider's exact UTF-8 version text. Empty, nil UUID, and +/// malformed bytes are not usable remote versions. +fn transitioned_version_from_meta_sys(meta_sys: &HashMap>) -> Result> { + if !contains_key_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_ID) { + return Ok(None); + } + let Some(value) = get_consistent_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_ID) else { + return Ok(None); + }; + let value = value.to_vec(); if value.is_empty() { - return None; + return Ok(None); } if let Ok(id) = Uuid::from_slice(&value) { - return (!id.is_nil()).then_some(id); + return Ok((!id.is_nil()).then(|| id.to_string())); } - std::str::from_utf8(&value) + let Ok(value) = String::from_utf8(value) else { + return Ok(None); + }; + if value.is_empty() + || value.len() > MAX_TRANSITION_VERSION_LEN + || value.chars().any(char::is_control) + || Uuid::parse_str(&value).is_ok_and(|id| id.is_nil()) + { + Ok(None) + } else { + Ok(Some(value)) + } +} + +fn transition_version_state_from_meta_sys( + meta_sys: &HashMap>, + version: Option<&str>, +) -> Result { + if !contains_key_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_STATE) { + return Ok(TransitionVersionState::Unknown); + } + let value = get_consistent_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_STATE).ok_or(Error::FileCorrupt)?; + let state = match value { + b"known-disabled" => TransitionVersionState::KnownDisabled, + b"suspended-null" => TransitionVersionState::SuspendedNull, + b"exact" => TransitionVersionState::Exact, + b"unknown" => TransitionVersionState::Unknown, + _ => return Err(Error::FileCorrupt), + }; + let valid = match state { + TransitionVersionState::Unknown | TransitionVersionState::KnownDisabled => version.is_none(), + TransitionVersionState::SuspendedNull => version == Some("null"), + TransitionVersionState::Exact => version.is_some_and(|value| value != "null"), + }; + valid.then_some(state).ok_or(Error::FileCorrupt) +} + +fn transition_version_state_bytes(state: TransitionVersionState) -> &'static [u8] { + match state { + TransitionVersionState::Unknown => b"unknown", + TransitionVersionState::KnownDisabled => b"known-disabled", + TransitionVersionState::SuspendedNull => b"suspended-null", + TransitionVersionState::Exact => b"exact", + } +} + +fn set_transition_version_state(meta_sys: &mut HashMap>, state: TransitionVersionState) { + if state == TransitionVersionState::Unknown { + remove_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_STATE); + } else { + insert_bytes( + meta_sys, + SUFFIX_TRANSITIONED_VERSION_STATE, + transition_version_state_bytes(state).to_vec(), + ); + } +} + +fn legacy_transitioned_version_id_from_meta_sys(meta_sys: &HashMap>) -> Option { + transitioned_version_from_meta_sys(meta_sys) .ok() - .and_then(|s| Uuid::parse_str(s.trim()).ok()) - .filter(|id| !id.is_nil()) + .flatten() + .and_then(|value| Uuid::parse_str(&value).ok()) +} + +fn transitioned_version_bytes(fi: &FileInfo) -> Option> { + fi.transition_version + .as_ref() + .map(|version| version.as_bytes().to_vec()) + .or_else(|| fi.transition_version_id.map(|version_id| version_id.as_bytes().to_vec())) } fn parse_legacy_erasure_algo(value: &str) -> ErasureAlgo { @@ -2398,7 +2470,9 @@ impl MetaObject { let transitioned_objname = get_bytes(&self.meta_sys, SUFFIX_TRANSITIONED_OBJECTNAME) .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); - let transition_version_id = transitioned_version_id_from_meta_sys(&self.meta_sys); + let transition_version = transitioned_version_from_meta_sys(&self.meta_sys)?; + let transition_version_state = transition_version_state_from_meta_sys(&self.meta_sys, transition_version.as_deref())?; + let transition_version_id = transition_version.as_deref().and_then(|value| Uuid::parse_str(value).ok()); let transition_tier = get_bytes(&self.meta_sys, SUFFIX_TRANSITION_TIER) .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); @@ -2419,6 +2493,8 @@ impl MetaObject { transition_status, transitioned_objname, transition_version_id, + transition_version, + transition_version_state, transition_tier, ..Default::default() }) @@ -2431,13 +2507,12 @@ impl MetaObject { SUFFIX_TRANSITIONED_OBJECTNAME, fi.transitioned_objname.as_bytes().to_vec(), ); - if let Some(transition_version_id) = fi.transition_version_id.as_ref() { - insert_bytes( - &mut self.meta_sys, - SUFFIX_TRANSITIONED_VERSION_ID, - transition_version_id.as_bytes().to_vec(), - ); + if let Some(transition_version) = transitioned_version_bytes(fi) { + insert_bytes(&mut self.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, transition_version); + } else { + remove_bytes(&mut self.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID); } + set_transition_version_state(&mut self.meta_sys, fi.transition_version_state); insert_bytes(&mut self.meta_sys, SUFFIX_TRANSITION_TIER, fi.transition_tier.as_bytes().to_vec()); if let Some(destination_id) = get_str(&fi.metadata, SUFFIX_TRANSITION_TIER_DESTINATION_ID) { insert_bytes(&mut self.meta_sys, SUFFIX_TRANSITION_TIER_DESTINATION_ID, destination_id.into_bytes()); @@ -2501,6 +2576,7 @@ impl MetaObject { SUFFIX_TRANSITION_TIER, SUFFIX_TRANSITIONED_OBJECTNAME, SUFFIX_TRANSITIONED_VERSION_ID, + SUFFIX_TRANSITIONED_VERSION_STATE, ] { if let Some(v) = get_bytes(&self.meta_sys, suffix) { insert_bytes(&mut delete_marker.meta_sys, suffix, v); @@ -2562,8 +2638,11 @@ impl From for MetaObject { ); } - if let Some(vid) = &value.transition_version_id { - insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, vid.as_bytes().to_vec()); + if let Some(transition_version) = transitioned_version_bytes(&value) { + insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, transition_version); + } + if !value.transition_status.is_empty() { + set_transition_version_state(&mut meta_sys, value.transition_version_state); } if !value.transition_tier.is_empty() { @@ -2706,7 +2785,11 @@ impl MetaDeleteMarker { .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); - fi.transition_version_id = transitioned_version_id_from_meta_sys(&self.meta_sys); + fi.transition_version = transitioned_version_from_meta_sys(&self.meta_sys).ok().flatten(); + fi.transition_version_id = legacy_transitioned_version_id_from_meta_sys(&self.meta_sys); + fi.transition_version_state = + transition_version_state_from_meta_sys(&self.meta_sys, fi.transition_version.as_deref()) + .unwrap_or(TransitionVersionState::Unknown); } fi @@ -2859,8 +2942,11 @@ impl From for MetaDeleteMarker { value.transitioned_objname.as_bytes().to_vec(), ); } - if let Some(version_id) = value.transition_version_id { - insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, version_id.as_bytes().to_vec()); + if let Some(transition_version) = transitioned_version_bytes(&value) { + insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, transition_version); + } + if !value.transition_status.is_empty() || value.tier_free_version() { + set_transition_version_state(&mut meta_sys, value.transition_version_state); } if !value.transition_tier.is_empty() { insert_bytes(&mut meta_sys, SUFFIX_TRANSITION_TIER, value.transition_tier.as_bytes().to_vec()); @@ -3412,7 +3498,7 @@ mod tests { .insert("x-rustfs-internal-healing".to_string(), "true".to_string()); marker.metadata.insert("content-type".to_string(), "text/plain".to_string()); let remote_version_id = Uuid::new_v4(); - marker.transition_version_id = Some(remote_version_id); + marker.transition_version = Some(remote_version_id.to_string()); let converted = MetaDeleteMarker::from(marker); @@ -3420,7 +3506,19 @@ mod tests { assert_eq!(converted.meta_sys.get("x-minio-internal-purgestatus"), Some(&b"pending".to_vec())); assert_eq!( get_bytes(&converted.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID), - Some(remote_version_id.as_bytes().to_vec()) + Some(remote_version_id.to_string().into_bytes()) + ); + assert_eq!( + converted + .meta_sys + .get(&format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_ID}")), + Some(&remote_version_id.to_string().into_bytes()) + ); + assert_eq!( + converted + .meta_sys + .get(&format!("{}{SUFFIX_TRANSITIONED_VERSION_ID}", rustfs_utils::http::MINIO_INTERNAL_PREFIX)), + Some(&remote_version_id.to_string().into_bytes()) ); assert!(!converted.meta_sys.contains_key("x-rustfs-internal-healing")); assert!(!converted.meta_sys.contains_key("content-type")); @@ -4097,19 +4195,129 @@ mod tests { .into_fileinfo("b", "k", false) .expect("into_fileinfo"); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); + assert_eq!(fi.transition_version_state, TransitionVersionState::Unknown); } #[test] - fn meta_object_transition_version_id_unparseable_stays_readable_as_none() { - // A non-UUID / non-16-byte tier version id must NOT make the object - // unreadable; it is tolerated as "no tier version" (compat with - // pre-hardening behavior and foreign/edge metadata). + fn meta_object_transition_version_id_opaque_text_is_preserved() { let mut sys = HashMap::new(); - insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"not-a-uuid".to_vec()); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"opaque-generation-42".to_vec()); let fi = make_meta_object_with_sys(sys) .into_fileinfo("b", "k", false) - .expect("unparseable transition version id must not fail the object read"); + .expect("opaque transition version id must decode"); assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version.as_deref(), Some("opaque-generation-42")); + assert_eq!(fi.transition_version_state, TransitionVersionState::Unknown); + } + + #[test] + fn meta_object_transition_version_state_exact_round_trips_dual_keys() { + let id = sample_version_id(); + let expected_version = id.to_string(); + let fi = FileInfo { + transition_status: "complete".to_string(), + transition_version: Some(expected_version.clone()), + transition_version_state: TransitionVersionState::Exact, + ..Default::default() + }; + + let object = MetaObject::from(fi); + assert_eq!( + object + .meta_sys + .get(&format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_STATE}")) + .map(Vec::as_slice), + Some(b"exact".as_slice()) + ); + assert_eq!( + object + .meta_sys + .get(&format!( + "{}{SUFFIX_TRANSITIONED_VERSION_STATE}", + rustfs_utils::http::MINIO_INTERNAL_PREFIX + )) + .map(Vec::as_slice), + Some(b"exact".as_slice()) + ); + assert_eq!( + legacy_transitioned_version_id_from_meta_sys(&object.meta_sys), + Some(id), + "UUID exact writes must remain readable by the legacy UUID consumer" + ); + let decoded = object.into_fileinfo("b", "k", false).expect("exact state should round trip"); + assert_eq!(decoded.transition_version_state, TransitionVersionState::Exact); + assert_eq!(decoded.transition_version.as_deref(), Some(expected_version.as_str())); + } + + #[test] + fn set_transition_known_disabled_removes_stale_version_dual_keys() { + let mut meta_sys = HashMap::new(); + insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, b"stale-legacy-version".to_vec()); + let mut object = make_meta_object_with_sys(meta_sys); + object.set_transition(&FileInfo { + transition_status: TRANSITION_COMPLETE.to_string(), + transitioned_objname: "remote/object".to_string(), + transition_version_state: TransitionVersionState::KnownDisabled, + transition_tier: "WARM".to_string(), + ..Default::default() + }); + + assert_eq!(get_bytes(&object.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID), None); + assert!( + !object + .meta_sys + .contains_key(&format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_ID}")) + ); + assert!( + !object + .meta_sys + .contains_key(&format!("{}{SUFFIX_TRANSITIONED_VERSION_ID}", rustfs_utils::http::MINIO_INTERNAL_PREFIX)) + ); + let decoded = object + .into_fileinfo("b", "k", false) + .expect("known-disabled transition must remain readable after replacing stale metadata"); + assert_eq!(decoded.transition_version, None); + assert_eq!(decoded.transition_version_state, TransitionVersionState::KnownDisabled); + } + + #[test] + fn meta_object_transition_version_state_conflict_fails_closed() { + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, sample_version_id().as_bytes().to_vec()); + sys.insert(format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_STATE}"), b"exact".to_vec()); + sys.insert( + format!("{}{SUFFIX_TRANSITIONED_VERSION_STATE}", rustfs_utils::http::MINIO_INTERNAL_PREFIX), + b"known-disabled".to_vec(), + ); + + make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect_err("conflicting state keys must fail closed"); + } + + #[test] + fn meta_object_transition_version_id_invalid_utf8_yields_none() { + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, vec![0xff]); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("invalid transition version bytes must not fail the object read"); + assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version, None); + } + + #[test] + fn meta_object_transition_version_id_unsafe_text_yields_none() { + for value in [b"opaque\0version".to_vec(), vec![b'x'; MAX_TRANSITION_VERSION_LEN + 1]] { + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, value); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("unsafe transition version text must not fail the object read"); + assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version, None); + } } #[test] @@ -4123,6 +4331,7 @@ mod tests { .into_fileinfo("b", "k", false) .expect("string-form transition version id must decode"); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); } #[test] @@ -4152,16 +4361,14 @@ mod tests { } .into_fileinfo("b", "k", false); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); } #[test] - fn delete_marker_free_version_transition_version_id_unparseable_stays_readable() { - // A malformed tier version id must not make a free-version record corrupt: - // it decodes to None and stays readable. Otherwise free-version expiry - // fails and the remote-tier object leaks. + fn delete_marker_free_version_transition_version_id_opaque_text_is_preserved() { let mut sys = HashMap::new(); insert_bytes(&mut sys, SUFFIX_FREE_VERSION, vec![]); - insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"not-a-uuid".to_vec()); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"opaque-generation-42".to_vec()); insert_bytes(&mut sys, SUFFIX_TRANSITION_TIER, b"WARM".to_vec()); insert_bytes(&mut sys, SUFFIX_TRANSITIONED_OBJECTNAME, b"remote-object".to_vec()); let fi = MetaDeleteMarker { @@ -4172,8 +4379,9 @@ mod tests { .into_fileinfo("b", "k", false); assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version.as_deref(), Some("opaque-generation-42")); fi.validate_for_metadata_read() - .expect("free-version record with an unparseable tier id must remain readable"); + .expect("free-version record with an opaque tier id must remain readable"); } #[test] @@ -4193,6 +4401,7 @@ mod tests { .into_fileinfo("b", "k", false); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); } #[test] diff --git a/crates/utils/src/http/metadata_compat.rs b/crates/utils/src/http/metadata_compat.rs index 9dc6ba175..c95f5b3d7 100644 --- a/crates/utils/src/http/metadata_compat.rs +++ b/crates/utils/src/http/metadata_compat.rs @@ -37,6 +37,7 @@ pub const SUFFIX_CRC: &str = "crc"; pub const SUFFIX_TRANSITION_STATUS: &str = "transition-status"; pub const SUFFIX_TRANSITIONED_OBJECTNAME: &str = "transitioned-object"; pub const SUFFIX_TRANSITIONED_VERSION_ID: &str = "transitioned-versionID"; +pub const SUFFIX_TRANSITIONED_VERSION_STATE: &str = "transitioned-version-state"; pub const SUFFIX_TRANSITION_TIER: &str = "transition-tier"; pub const SUFFIX_TRANSITION_TIER_DESTINATION_ID: &str = "transition-tier-destination-id"; pub const SUFFIX_RESTORE_OPERATION_ID: &str = "restore-operation-id"; diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index 6e04bef5c..e77f7c0e6 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -763,7 +763,7 @@ async fn enqueue_transitioned_delete_cleanup( let _activity_guard = DeleteTailActivityGuard::new(DeleteTailStage::Cleanup); let je = if opts.delete_prefix { - tier_sweeper::transitioned_force_delete_journal_entry(&existing.transitioned_object) + tier_sweeper::transitioned_force_delete_journal_entry(&existing.transitioned_object, existing.transition_version_state) } else { let version_id = opts.version_id.as_ref().and_then(|v| Uuid::parse_str(v).ok()); tier_sweeper::transitioned_delete_journal_entry( @@ -771,6 +771,7 @@ async fn enqueue_transitioned_delete_cleanup( opts.versioned, opts.version_suspended, &existing.transitioned_object, + existing.transition_version_state, ) }; let Some(mut je) = je else { @@ -9683,7 +9684,7 @@ mod tests { #[tokio::test] #[serial_test::serial] - async fn transitioned_delete_cleanup_persists_identity_bound_and_legacy_journals() { + async fn transitioned_delete_cleanup_persists_known_state_and_rejects_unknown_state() { let store = crate::app::gating_test_env::shared_gating_ecstore().await; if current_app_context().is_none() { crate::app::runtime_sources::install_test_app_context(Arc::clone(&store)).await; @@ -9703,8 +9704,9 @@ mod tests { current.transitioned_object.tier = "WARM".to_string(); current.transitioned_object.name = "remote/identity-bound".to_string(); current.transitioned_object.version_id = "remote-version".to_string(); + current.transition_version_state = rustfs_filemeta::TransitionVersionState::Exact; - let journal_name = |remote_object: &str, backend_identity: Option<[u8; 32]>| { + let journal_name = |remote_object: &str, backend_identity: Option<[u8; 32]>, version_id_exact: bool| { use sha2::{Digest, Sha256}; let mut hasher = Sha256::new(); @@ -9717,6 +9719,10 @@ mod tests { hasher.update([0]); hasher.update(backend_identity); } + if version_id_exact { + hasher.update([0]); + hasher.update(b"exact-version-id"); + } format!("ilm/tier-delete-journal/{}.json", rustfs_utils::crypto::hex(hasher.finalize().as_slice())) }; @@ -9726,7 +9732,7 @@ mod tests { let mut identity_bound = store .get_object_reader( ".rustfs.sys", - &journal_name("remote/identity-bound", Some(identity)), + &journal_name("remote/identity-bound", Some(identity), true), None, http::HeaderMap::new(), &ObjectOptions::default(), @@ -9739,11 +9745,14 @@ mod tests { .expect("identity-bound journal body should be readable"); let identity_bound: serde_json::Value = serde_json::from_slice(&identity_bound_data).expect("identity-bound journal should decode as JSON"); - assert_eq!(identity_bound["version"], serde_json::json!(2)); + assert_eq!(identity_bound["version"], serde_json::json!(4)); assert_eq!(identity_bound["backend_identity"], serde_json::json!(identity)); + assert_eq!(identity_bound["version_id_exact"], serde_json::json!(true)); + assert_eq!(identity_bound["version_state"], serde_json::json!("exact")); current.user_defined = Arc::new(HashMap::new()); current.transitioned_object.name = "remote/legacy".to_string(); + current.transition_version_state = rustfs_filemeta::TransitionVersionState::Unknown; enqueue_transitioned_delete_cleanup( store.clone(), "bucket", @@ -9755,24 +9764,31 @@ mod tests { Some(¤t), ) .await - .expect("legacy force-delete cleanup should persist a fail-closed v1 journal"); - let mut legacy = store + .expect("unknown force-delete cleanup should fail closed without a journal"); + let legacy_err = match store .get_object_reader( ".rustfs.sys", - &journal_name("remote/legacy", None), + &journal_name("remote/legacy", None, false), None, http::HeaderMap::new(), &ObjectOptions::default(), ) .await - .expect("legacy journal should be readable"); - let mut legacy_data = Vec::new(); - tokio::io::AsyncReadExt::read_to_end(&mut legacy.stream, &mut legacy_data) - .await - .expect("legacy journal body should be readable"); - let legacy: serde_json::Value = serde_json::from_slice(&legacy_data).expect("legacy journal should decode as JSON"); - assert_eq!(legacy["version"], serde_json::json!(1)); - assert_eq!(legacy["backend_identity"], serde_json::Value::Null); + { + Ok(_) => panic!("unknown remote version state must not persist a delete journal"), + Err(err) => err, + }; + assert!( + matches!( + &legacy_err, + StorageError::FileNotFound + | StorageError::ObjectNotFound(_, _) + | StorageError::FileVersionNotFound + | StorageError::VersionNotFound(_, _, _) + | StorageError::VolumeNotFound + ), + "unknown remote version state must leave no journal, got {legacy_err:?}" + ); } async fn put_real_cold_fill_object(store: &Arc, bucket: &str, object: &str, body: &[u8]) -> ObjectInfo { diff --git a/rustfs/src/app/storage_api.rs b/rustfs/src/app/storage_api.rs index 06765ac96..b97477b9a 100644 --- a/rustfs/src/app/storage_api.rs +++ b/rustfs/src/app/storage_api.rs @@ -409,20 +409,24 @@ pub(crate) mod bucket { versioned: bool, suspended: bool, transitioned: &super::super::super::storage_contracts::TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, ) -> Option { crate::storage::storage_api::ecstore_bucket::lifecycle::tier_sweeper::transitioned_delete_journal_entry( version_id, versioned, suspended, transitioned, + transition_version_state, ) } pub(crate) fn transitioned_force_delete_journal_entry( transitioned: &super::super::super::storage_contracts::TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, ) -> Option { crate::storage::storage_api::ecstore_bucket::lifecycle::tier_sweeper::transitioned_force_delete_journal_entry( transitioned, + transition_version_state, ) } } From 2423ba8e3f89170f88429035e4c4f092188f5965 Mon Sep 17 00:00:00 2001 From: cxymds Date: Wed, 29 Jul 2026 10:55:00 +0800 Subject: [PATCH 7/8] fix(tiering): base restore expiry on completion (#5366) * fix(tiering): base restore expiry on completion * fix(tiering): import restore metadata types * fix(restore): resolve metadata finalization build errors * test(ecstore): import restore expiry helper --- crates/ecstore/src/set_disk/ops/object.rs | 2 + crates/ecstore/src/set_disk/replication.rs | 66 +++++++++++++++++++ .../src/set_disk/transition_matrix_tests.rs | 55 ++++++++++++++-- 3 files changed, 118 insertions(+), 5 deletions(-) diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 41775da3e..0236b7cd0 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -4185,6 +4185,7 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { let mut p_reader = PutObjReader::new(hash_reader); return match self_.clone().put_object(bucket, object, &mut p_reader, &ropts).await { Ok(restored_info) => { + let restored_info = self_.finalize_restore_metadata(bucket, object, &restored_info, &opts).await?; send_event(EventArgs { event_name: EventName::ObjectRestoreCompleted.as_str().to_string(), bucket_name: bucket.to_string(), @@ -4319,6 +4320,7 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { return set_restore_header_fn(&mut oi, Some(err)).await; } }; + let restored_info = self_.finalize_restore_metadata(bucket, object, &restored_info, opts).await?; send_event(EventArgs { event_name: EventName::ObjectRestoreCompleted.as_str().to_string(), bucket_name: bucket.to_string(), diff --git a/crates/ecstore/src/set_disk/replication.rs b/crates/ecstore/src/set_disk/replication.rs index 611abeb7a..ad84d4842 100644 --- a/crates/ecstore/src/set_disk/replication.rs +++ b/crates/ecstore/src/set_disk/replication.rs @@ -13,7 +13,10 @@ // limitations under the License. use super::*; +use crate::bucket::lifecycle::lifecycle; +use rustfs_filemeta::RestoreStatusOps; use rustfs_utils::http::headers::{AMZ_RESTORE_EXPIRY_DAYS, AMZ_RESTORE_REQUEST_DATE}; +use s3s::dto::{RestoreStatus, Timestamp}; #[derive(Clone, Copy, Debug, Eq, PartialEq)] struct RestoreCleanupIdentity { @@ -43,6 +46,69 @@ impl RestoreCleanupIdentity { } impl SetDisks { + pub(super) async fn finalize_restore_metadata( + &self, + bucket: &str, + object: &str, + obj_info: &ObjectInfo, + opts: &ObjectOptions, + ) -> Result { + let expected = RestoreCleanupIdentity::from_object_info(obj_info); + let expected_operation_id = restore_operation_id_from_metadata(&opts.user_defined)?; + let expected_etag = obj_info + .etag + .clone() + .unwrap_or_else(|| get_raw_etag(obj_info.user_defined.as_ref())); + let version_id = expected.version_id.map(|v| v.to_string()); + let _lock_guard = if !opts.no_lock { + Some( + self.acquire_write_lock_diag("restore_finalize_metadata", bucket, object) + .await?, + ) + } else { + None + }; + let read_opts = ObjectOptions { + version_id, + versioned: opts.versioned, + version_suspended: opts.version_suspended, + ..Default::default() + }; + let (mut fi, _, disks) = self + .get_object_fileinfo_gated(bucket, object, &read_opts, false, false) + .await?; + if let Some(expected_operation_id) = expected_operation_id { + require_restore_operation_id(&fi.metadata, expected_operation_id)?; + } + if !expected.matches_file_info(&fi, &expected_etag) { + return Err(Error::other("restored object changed before restore metadata finalization")); + } + let restore_expiry = + lifecycle::expected_expiry_time(OffsetDateTime::now_utc(), opts.transition.restore_request.days.unwrap_or(1)); + fi.metadata.insert( + X_AMZ_RESTORE.as_str().to_string(), + RestoreStatus { + is_restore_in_progress: Some(false), + restore_expiry_date: Some(Timestamp::from(restore_expiry)), + } + .to_string(), + ); + self.invalidate_get_object_metadata_cache(bucket, object).await; + self.update_object_meta_with_opts( + bucket, + object, + fi.clone(), + disks.as_slice(), + &UpdateMetadataOpts { + replace_user_metadata: true, + ..Default::default() + }, + ) + .await?; + self.invalidate_get_object_metadata_cache(bucket, object).await; + Ok(ObjectInfo::from_file_info(&fi, bucket, object, opts.versioned || opts.version_suspended)) + } + pub async fn update_restore_metadata( &self, bucket: &str, diff --git a/crates/ecstore/src/set_disk/transition_matrix_tests.rs b/crates/ecstore/src/set_disk/transition_matrix_tests.rs index e77a0e437..a137d55a6 100644 --- a/crates/ecstore/src/set_disk/transition_matrix_tests.rs +++ b/crates/ecstore/src/set_disk/transition_matrix_tests.rs @@ -13,10 +13,11 @@ // limitations under the License. use super::*; -use crate::bucket::lifecycle::lifecycle::{TRANSITION_COMPLETE, TRANSITION_PENDING, TransitionOptions}; +use crate::bucket::lifecycle::lifecycle::{TRANSITION_COMPLETE, TRANSITION_PENDING, TransitionOptions, expected_expiry_time}; use crate::ecstore_validation_blackbox::make_local_set_disks; use crate::services::tier::test_util::register_mock_tier; use crate::storage_api_contracts::object::{ObjectIO as _, ObjectOperations as _}; +use rustfs_filemeta::{RestoreStatusOps as _, parse_restore_obj_status}; use tokio::io::AsyncReadExt; async fn prime_metadata_generation(set_disks: &SetDisks, bucket: &str, object: &str) -> GetObjectMetadataCacheKey { @@ -83,13 +84,57 @@ async fn transition_and_restore_reclaim_prior_metadata_generations() { let transitioned_generation = prime_metadata_generation(&set_disks, bucket, object).await; let mut restore_opts = ObjectOptions::default(); restore_opts.transition.restore_request.days = Some(1); - Arc::clone(&set_disks) - .restore_transitioned_object(bucket, object, &restore_opts) - .await - .expect("restore should succeed"); + let restore_started = OffsetDateTime::now_utc(); + let expiry_from_restore_start = temp_env::async_with_vars( + [ + ("RUSTFS_ILM_DEBUG_DAY_SECS", Some("1")), + ("RUSTFS_ILM_PROCESS_TIME", Some("1")), + ], + async { + let expiry_from_restore_start = expected_expiry_time(restore_started, 1); + let get_barrier = backend.arm_get_barrier().await; + let restore_set = Arc::clone(&set_disks); + let restore = + tokio::spawn(async move { restore_set.restore_transitioned_object(bucket, object, &restore_opts).await }); + get_barrier.wait_until_paused().await; + tokio::time::timeout(Duration::from_secs(5), async { + loop { + if expected_expiry_time(OffsetDateTime::now_utc(), 1) > expiry_from_restore_start { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("test clock should cross the next accelerated lifecycle boundary"); + get_barrier.release(); + restore + .await + .expect("restore task should join") + .expect("restore should succeed"); + expiry_from_restore_start + }, + ) + .await; assert_generation_reclaimed(&set_disks, &transitioned_generation).await; assert_eq!(backend.get_count().await, 1, "restore should read the remote candidate exactly once"); + let restored_info = set_disks + .get_object_info(bucket, object, &ObjectOptions::default()) + .await + .expect("restored object metadata should be readable"); + let restore_status = parse_restore_obj_status( + restored_info + .user_defined + .get(s3s::header::X_AMZ_RESTORE.as_str()) + .expect("completed restore header should be present"), + ) + .expect("completed restore header should parse"); + assert!( + restore_status.expiry().expect("completed restore should have an expiry") > expiry_from_restore_start, + "restore expiry must be based on completion, not the time the remote copy started" + ); + let mut restored = Vec::new(); set_disks .get_object_reader(bucket, object, None, HeaderMap::new(), &ObjectOptions::default()) From c2f503b28c0e2c16601d06ff11786c6997d7007e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=A9=AC=E7=99=BB=E5=B1=B1?= Date: Wed, 29 Jul 2026 13:16:08 +0800 Subject: [PATCH 8/8] fix(rpc): keep snapshot lease checks CI-compatible --- .../e2e_test/src/reliant/grpc_lock_server.rs | 24 ++++++++++++++++- rustfs/src/storage/rpc/node_service/disk.rs | 26 ++++++++----------- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/crates/e2e_test/src/reliant/grpc_lock_server.rs b/crates/e2e_test/src/reliant/grpc_lock_server.rs index 301125188..76284ad2d 100644 --- a/crates/e2e_test/src/reliant/grpc_lock_server.rs +++ b/crates/e2e_test/src/reliant/grpc_lock_server.rs @@ -23,7 +23,8 @@ use rustfs_protos::{ proto_gen::node_service::{ BatchGenerallyLockRequest, BatchGenerallyLockResponse, BatchReadVersionRequest, BatchReadVersionResponse, GenerallyLockRequest, GenerallyLockResponse, GenerallyLockResult, PingRequest, PingResponse, - node_service_server::NodeService, + SnapshotLeaseMutationResponse, SnapshotLeaseReleaseRequest, SnapshotLeaseRenewRequest, SnapshotLeaseRequest, + SnapshotLeaseResponse, node_service_server::NodeService, }, }; use std::pin::Pin; @@ -104,6 +105,27 @@ impl NodeService for MinimalLockNodeService { Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) } + async fn acquire_snapshot_lease( + &self, + _request: Request, + ) -> Result, Status> { + Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) + } + + async fn renew_snapshot_lease( + &self, + _request: Request, + ) -> Result, Status> { + Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) + } + + async fn release_snapshot_lease( + &self, + _request: Request, + ) -> Result, Status> { + Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) + } + async fn lock(&self, request: Request) -> Result, Status> { let request = request.into_inner(); let args: LockRequest = match serde_json::from_str(&request.args) { diff --git a/rustfs/src/storage/rpc/node_service/disk.rs b/rustfs/src/storage/rpc/node_service/disk.rs index ca863d0e5..d5fa44815 100644 --- a/rustfs/src/storage/rpc/node_service/disk.rs +++ b/rustfs/src/storage/rpc/node_service/disk.rs @@ -120,19 +120,6 @@ fn snapshot_lease_ttl(ttl_ms: u64) -> Result { Ok(ttl) } -#[cfg(test)] -mod snapshot_lease_tests { - use super::{SNAPSHOT_LEASE_MAX_TTL, SNAPSHOT_LEASE_MIN_TTL, snapshot_lease_ttl}; - - #[test] - fn snapshot_lease_ttl_rejects_values_outside_server_bounds() { - assert!(snapshot_lease_ttl(4_999).is_err()); - assert_eq!(snapshot_lease_ttl(5_000).unwrap(), SNAPSHOT_LEASE_MIN_TTL); - assert_eq!(snapshot_lease_ttl(300_000).unwrap(), SNAPSHOT_LEASE_MAX_TTL); - assert!(snapshot_lease_ttl(300_001).is_err()); - } -} - fn decode_msgpack_or_json( binary: &[u8], json: &str, @@ -1605,8 +1592,9 @@ impl NodeService { #[cfg(test)] mod tests { use super::{ - compat_response_json, decode_msgpack_or_json, encode_batch_read_version_response_payloads, encode_msgpack, - encode_msgpack_named, encode_read_multiple_response_payloads, + SNAPSHOT_LEASE_MAX_TTL, SNAPSHOT_LEASE_MIN_TTL, compat_response_json, decode_msgpack_or_json, + encode_batch_read_version_response_payloads, encode_msgpack, encode_msgpack_named, + encode_read_multiple_response_payloads, snapshot_lease_ttl, }; use crate::storage::storage_api::ReadMultipleResp; use crate::storage::storage_api::rpc_consumer::node_service::BatchReadVersionResp; @@ -1619,6 +1607,14 @@ mod tests { count: u32, } + #[test] + fn snapshot_lease_ttl_rejects_values_outside_server_bounds() { + assert!(snapshot_lease_ttl(4_999).is_err()); + assert_eq!(snapshot_lease_ttl(5_000).unwrap(), SNAPSHOT_LEASE_MIN_TTL); + assert_eq!(snapshot_lease_ttl(300_000).unwrap(), SNAPSHOT_LEASE_MAX_TTL); + assert!(snapshot_lease_ttl(300_001).is_err()); + } + #[test] fn decode_msgpack_or_json_prefers_binary_payload() { let payload = SamplePayload {