Skip to content

fix(notify): emit delete webhooks for prefix deletes and align replication headers#8

Open
andybrown668 wants to merge 68 commits intomainfrom
fix-missing-delete-webhook-notification
Open

fix(notify): emit delete webhooks for prefix deletes and align replication headers#8
andybrown668 wants to merge 68 commits intomainfrom
fix-missing-delete-webhook-notification

Conversation

@andybrown668
Copy link
Copy Markdown

Type of Change

  • New Feature
  • Bug Fix
  • Documentation
  • Performance Improvement
  • Test/CI
  • Refactor
  • Other:

Related Issues

N/A

Summary of Changes

Emit delete webhooks for prefix deletes when ObjectInfo is empty so bucket notifications match S3 DELETE behavior. Run delete-notification work via spawn_background and export the helper from storage.

Treat replication request headers like del_opts: only true / 1 count as enabled, and honor x-minio-source-replication-request. Add a regression test for header semantics.

Update scripts/run.sh after merge: jemalloc handling for rustfs-only and console URL helpers.

Checklist

  • I have read and followed the CONTRIBUTING.md guidelines
  • Passed make pre-commit
  • Added/updated necessary tests
  • Documentation updated (if needed)
  • CI/CD passed (if applicable)

Impact

  • Breaking change (compatibility)
  • Requires doc/config/deployment update
  • Other impact: Notification and replication header behavior fixes only.

Additional Notes

N/A

Verification

make pre-commit

Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md). If this is your first contribution, review the CLA document and sign it by commenting I have read and agree to the CLA. on the PR.

polds and others added 30 commits March 24, 2026 09:54
Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: cxymds <Cxymds@qq.com>
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: loverustfs <hello@rustfs.com>
…2275)

Signed-off-by: heihutu <heihutu@gmail.com>
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: cxymds <Cxymds@qq.com>
Co-authored-by: cxymds <Cxymds@qq.com>
Co-authored-by: loverustfs <hello@rustfs.com>
Co-authored-by: heihutu <heihutu@gmail.com>
…s#2285)

Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: houseme <4829346+houseme@users.noreply.github.com>
Co-authored-by: weisd <im@weisd.in>
Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: houseme <housemecn@gmail.com>
Signed-off-by: houseme <housemecn@gmail.com>
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: 安正超 <anzhengchao@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: houseme <4829346+houseme@users.noreply.github.com>
…on checks (rustfs#2315)

Signed-off-by: GatewayJ <835269233@qq.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: GatewayJ <8352692332qq.com>
lunrenyi and others added 18 commits March 30, 2026 22:05
Co-authored-by: 安正超 <anzhengchao@gmail.com>
Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: cxymds <Cxymds@qq.com>
Co-authored-by: cxymds <Cxymds@qq.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: houseme <housemecn@gmail.com>
Co-authored-by: heihutu <heihutu@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: 安正超 <anzhengchao@gmail.com>
Signed-off-by: 安正超 <anzhengchao@gmail.com>
Signed-off-by: 安正超 <anzhengchao@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@andybrown668 andybrown668 force-pushed the fix-missing-delete-webhook-notification branch from f235f35 to f21ad46 Compare April 3, 2026 11:06
Copilot AI review requested due to automatic review settings April 3, 2026 11:52
@andybrown668 andybrown668 force-pushed the fix-missing-delete-webhook-notification branch 2 times, most recently from e3ae816 to 8ac2c80 Compare April 3, 2026 11:56
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to align RustFS behavior with S3/MinIO semantics across notifications, replication headers, and lifecycle/replication workflows, while also introducing a broader set of storage, I/O, test, and CI enhancements (notably zero-copy reads, new concurrency/io crates, and additional E2E regressions).

Changes:

  • Fix/extend object metadata parsing + add legacy compatibility tests (xl.meta and version handling).
  • Introduce/propagate zero-copy read support (DiskAPI + bitrot reader), consolidate tier transition PutObjectOptions, and add related config/constants.
  • Expand operational/test surface (replication resync cancellation tokens, richer notification error aggregation, new E2E regressions, CLA workflow + build flags).

Reviewed changes

Copilot reviewed 105 out of 299 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
crates/filemeta/src/filemeta/codec.rs Use header version when unmarshalling legacy xl.meta headers.
crates/filemeta/src/filemeta.rs Add legacy xl.meta compatibility tests and adjust version struct initialization.
crates/ecstore/tests/legacy_bitrot_read_test.rs Update bitrot reader calls to include zero-copy parameter.
crates/ecstore/src/tier/warm_backend_tencent.rs Reuse shared transition put options builder for Tencent backend.
crates/ecstore/src/tier/warm_backend_s3.rs Reuse shared transition put options builder for S3 backend.
crates/ecstore/src/tier/warm_backend_rustfs.rs Reuse shared transition put options builder for RustFS tier backend.
crates/ecstore/src/tier/warm_backend_r2.rs Reuse shared transition put options builder for R2 backend.
crates/ecstore/src/tier/warm_backend_minio.rs Reuse shared transition put options builder for MinIO tier backend.
crates/ecstore/src/tier/warm_backend_huaweicloud.rs Reuse shared transition put options builder for HuaweiCloud backend.
crates/ecstore/src/tier/warm_backend_azure.rs Reuse shared transition put options builder for Azure backend.
crates/ecstore/src/tier/warm_backend_aliyun.rs Reuse shared transition put options builder for Aliyun backend.
crates/ecstore/src/tier/warm_backend.rs Add build_transition_put_options helper + tests for promoted headers.
crates/ecstore/src/tier/tier.rs Improve tier config “not initialized” error messaging + unit test.
crates/ecstore/src/store_api/types.rs Extend ObjectOptions with incl_free_versions option.
crates/ecstore/src/store_api/readers.rs Simplify HashReader setup via from_stream helper.
crates/ecstore/src/store_api.rs Adjust imports after HashReader construction change.
crates/ecstore/src/store.rs Enqueue lifecycle transitions immediately after successful writes; add test.
crates/ecstore/src/set_disk/read.rs Thread incl_free_versions option and add zero-copy selection via env flag.
crates/ecstore/src/set_disk/multipart.rs Update read_all_fileinfo invocation with new parameter.
crates/ecstore/src/set_disk/heal.rs Update heal read path for new read_all_fileinfo signature and zero-copy.
crates/ecstore/src/rpc/remote_disk.rs Implement read_file_zero_copy for remote disks (efficient fallback).
crates/ecstore/src/rpc/peer_rest_client.rs Add live events RPC client types/method.
crates/ecstore/src/notification_sys.rs Return Results from peer actions; aggregate peer failures with context + tests.
crates/ecstore/src/lib.rs Add internal data_movement module.
crates/ecstore/src/event_notification.rs Add global event dispatch hook registration + test.
crates/ecstore/src/error.rs Add rebalance/cancel errors and helper predicates + tests.
crates/ecstore/src/disk/mod.rs Add DiskAPI read_file_zero_copy method and docs.
crates/ecstore/src/disk/local.rs Implement local read_file_zero_copy via mmap/reads + overflow safety + tests.
crates/ecstore/src/disk/disk_store.rs Wrap local read_file_zero_copy with disk health tracking.
crates/ecstore/src/bucket/replication/replication_resyncer.rs Replace global cancel token with per-bucket/target tokens; stale update guard; tests.
crates/ecstore/src/bucket/replication/replication_pool.rs Expose resync APIs; spawn resync tasks with per-run cancel tokens.
crates/ecstore/src/bucket/object_lock/objectlock_sys.rs Block retention mode changes correctly; update tests.
crates/ecstore/src/bucket/metadata_sys.rs Add getters for website/logging/accelerate/request-payment configs.
crates/ecstore/src/bucket/metadata.rs Extend BucketMetadata to store/parse new bucket configs and timestamps.
crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs Export journal entry helpers for transitioned object cleanup.
crates/ecstore/src/bucket/lifecycle/rule.rs Implement lifecycle tag/size filtering and add unit tests.
crates/ecstore/src/bucket/lifecycle/lifecycle.rs Apply filter logic in lifecycle rule selection and extend tests.
crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs Implement remote tier delete handling for transition cleanup; adjust event names; tests.
crates/ecstore/src/bucket/bucket_target_sys.rs Improve S3 client error metadata capture and structured constructors.
crates/ecstore/src/bitrot.rs Add optional zero-copy reads, metrics, inline offset behavior fix, and tests.
crates/ecstore/Cargo.toml Add memmap2 + io-metrics workspace deps; remove moka dep entry.
crates/e2e_test/src/version_id_regression_test.rs Update semantics: omit version_id when versioning suspended; add tests for Copy/MPU.
crates/e2e_test/src/reliant/lock.rs Add more distributed-lock reliability tests and helpers.
crates/e2e_test/src/reliant/grpc_lock_server.rs Stub get_live_events for lock-only test node service.
crates/e2e_test/src/quota_test.rs Skip quota tests when awscurl is unavailable.
crates/e2e_test/src/protocols/webdav_core.rs Use feature-aware binary path for WebDAV tests.
crates/e2e_test/src/protocols/ftps_core.rs Update FTPS test TLS handling and binary feature selection.
crates/e2e_test/src/policy/policy_variables_test.rs Use DELETE for cleanup endpoints via awscurl_delete.
crates/e2e_test/src/list_objects_v2_metadata_extension_test.rs Add E2E test for list-type=2&metadata=true extension.
crates/e2e_test/src/list_object_versions_metadata_extension_test.rs Add E2E test for versions&metadata=true extension.
crates/e2e_test/src/lib.rs Register new E2E modules.
crates/e2e_test/src/kms/multipart_encryption_test.rs Use shared SSE-C MD5 base64 helper.
crates/e2e_test/src/kms/kms_vault_test.rs Skip KMS tests when admin tooling missing; use SSE-C MD5 base64 helper.
crates/e2e_test/src/kms/kms_local_test.rs Same: tooling skip + SSE-C MD5 base64 helper.
crates/e2e_test/src/kms/kms_edge_cases_test.rs Same: SSE-C MD5 base64 helper.
crates/e2e_test/src/kms/kms_comprehensive_test.rs Same: SSE-C MD5 base64 helper.
crates/e2e_test/src/kms/encryption_metadata_test.rs Assert managed encryption metadata is not exposed to clients.
crates/e2e_test/src/kms/common.rs Add tooling skip + SSE-C MD5 base64 helper; tighten HTTP client usage.
crates/e2e_test/src/checksum_upload_test.rs Add CRC64NVME parity regression test and checksum utilities.
crates/e2e_test/src/bucket_policy_check_test.rs Skip policy test when awscurl is unavailable.
crates/e2e_test/src/anonymous_access_test.rs Use local HTTP client helper for anonymous requests.
crates/e2e_test/Cargo.toml Add dependencies required by new E2E tests (rio/signer/http/etc.).
crates/config/src/lib.rs Re-export new constants modules.
crates/config/src/constants/zero_copy.rs Add zero-copy/direct I/O env constants and defaults.
crates/config/src/constants/runtime.rs Add dial9 tokio telemetry env constants and defaults.
crates/config/src/constants/mod.rs Register new constants modules.
crates/config/src/constants/capacity.rs Add capacity-calculation config constants + tests.
crates/concurrency/src/*.rs Introduce new rustfs-concurrency crate with timeout/lock/backpressure/scheduler/deadlock modules.
crates/concurrency/Cargo.toml Add new workspace crate manifest and feature flags.
crates/checksums/src/lib.rs Fix typo in test assertion message.
README_ZH.md Document Nix flake and X-CMD install options (ZH).
README.md Document X-CMD install option (EN).
Dockerfile Add zlib dependency and change apk installation approach.
Cargo.toml Add new workspace crates and bump multiple dependency versions.
AGENTS.md Add PR verification wording and style guidance around helpers/strings.
.github/workflows/cla.yml Add CLA check workflow.
.github/workflows/build.yml Ensure tokio_unstable cfg is always included in CI RUSTFLAGS.
.github/pull_request_template.md Update CLA signing instructions.
.github/cla.yml Add CLA bot configuration file.
.cargo/config.toml Add tokio_unstable rustflags for dial9 telemetry.
.agents/skills/pr-creation-checker/* Update PR readiness checklist for string literal reuse.

Comment on lines 40 to +42
"crates/protos", # Protocol buffer definitions
"crates/rio", # Rust I/O utilities and abstractions
"crates/concurrency", # Rust I/O utilities and abstractions
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title/description focuses on notify delete webhooks + replication headers, but the diff also adds new workspace crates (concurrency/io-core/io-metrics), CI/tooling/workflows, Docker changes, and multiple unrelated e2e/regression areas. This scope mismatch makes review and rollback riskier. Consider splitting into focused PRs (e.g., notify/replication vs zero-copy I/O vs CLA/CI/docs), or update the PR description to explicitly enumerate these additional changes and their motivation.

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +53
"crates/io-metrics", # Zero-copy metrics collection for performance analysis
"crates/io-core", # Zero-copy core reader and writer implementations
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title/description focuses on notify delete webhooks + replication headers, but the diff also adds new workspace crates (concurrency/io-core/io-metrics), CI/tooling/workflows, Docker changes, and multiple unrelated e2e/regression areas. This scope mismatch makes review and rollback riskier. Consider splitting into focused PRs (e.g., notify/replication vs zero-copy I/O vs CLA/CI/docs), or update the PR description to explicitly enumerate these additional changes and their motivation.

Copilot uses AI. Check for mistakes.
Comment on lines +88 to +108
static EVENT_DISPATCH_HOOK: OnceLock<EventDispatchHook> = OnceLock::new();

pub fn register_event_dispatch_hook<F>(hook: F) -> bool
where
F: Fn(EventArgs) + Send + Sync + 'static,
{
EVENT_DISPATCH_HOOK.set(Arc::new(hook)).is_ok()
}

pub fn send_event(args: EventArgs) {
if let Some(hook) = EVENT_DISPATCH_HOOK.get() {
hook(args);
return;
}

warn!(
event_name = args.event_name,
bucket = args.bucket_name,
"event send() dropped because no event dispatch hook is registered"
);
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

send_event() logs a warn for every dropped event when no hook is registered. If send_event is called on hot paths during startup/misconfiguration, this can flood logs and obscure other issues. Consider either (a) logging this at debug level, (b) rate-limiting it (e.g., log once via a OnceLock/AtomicBool), or (c) returning a Result so callers can decide how to handle missing dispatch.

Copilot uses AI. Check for mistakes.
Comment on lines +869 to +874
#[instrument(skip(_cancellation_token))]
async fn load_resync(
self: Arc<Self>,
buckets: &[String],
_cancellation_token: CancellationToken,
) -> Result<(), EcstoreError> {
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_resync() now ignores the passed cancellation token and spawns resync tasks using fresh CancellationTokens. This prevents a top-level shutdown/cancel from propagating to in-flight resync tasks and can lead to leaked background work. Prefer deriving per-task tokens from the provided token (e.g., use a child token if available, or select on both tokens) so the replication pool can reliably stop/tear down.

Copilot uses AI. Check for mistakes.
// For now, we just log the resync request

let ctx = cancellation_token.clone();
let ctx = CancellationToken::new();
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_resync() now ignores the passed cancellation token and spawns resync tasks using fresh CancellationTokens. This prevents a top-level shutdown/cancel from propagating to in-flight resync tasks and can lead to leaked background work. Prefer deriving per-task tokens from the provided token (e.g., use a child token if available, or select on both tokens) so the replication pool can reliably stop/tear down.

Copilot uses AI. Check for mistakes.
Comment on lines +502 to +504
/// Zero-copy file read using memory mapping (Unix) or efficient read (non-Unix).
/// Returns Bytes that can be shared without copying.
/// On Unix, this uses mmap for true zero-copy access.
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trait docs claim "true zero-copy" via mmap on Unix and "shared without copying", but the LocalDisk implementation currently copies the mmap region into a Bytes buffer (Bytes::copy_from_slice). Please update the DiskAPI docs to match actual behavior (e.g., 'mmap-assisted read, may still copy into Bytes for ownership/safety'), or adjust the implementation to actually return a Bytes backed by the mmap region with safe ownership semantics.

Suggested change
/// Zero-copy file read using memory mapping (Unix) or efficient read (non-Unix).
/// Returns Bytes that can be shared without copying.
/// On Unix, this uses mmap for true zero-copy access.
/// Read a file range using platform-specific optimizations.
/// Implementations may use memory mapping on Unix as a read optimization,
/// but the returned `Bytes` may still be backed by copied data to provide
/// safe ownership and lifetime semantics.

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +76
RUN apk update && \
apk add --no-cache ca-certificates coreutils curl "zlib>=1.3.2-r0"
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using apk update adds an extra layer and is usually unnecessary when apk add --no-cache is used. Also, pinning/constrainting zlib as \"zlib>=1.3.2-r0\" can break builds depending on the Alpine repository snapshot used by the base image. Consider removing apk update and installing zlib without a version constraint (or pin the base image digest/repository) to keep Docker builds reproducible and less brittle.

Suggested change
RUN apk update && \
apk add --no-cache ca-certificates coreutils curl "zlib>=1.3.2-r0"
RUN apk add --no-cache ca-certificates coreutils curl zlib

Copilot uses AI. Check for mistakes.
Comment on lines +114 to +116
if let Err(e) = config.validate() {
panic!("Invalid concurrency configuration: {}", e);
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConcurrencyManager::new() panics on invalid configuration. Since this is a library-style crate and config values can come from env/user input, panicking makes error handling harder for callers and can crash long-running services. Consider adding a try_new(...) -> Result<Self, ConfigError> (or returning Result from new) and keeping new as a convenience wrapper only if you strongly want a panic-on-invalid default.

Copilot uses AI. Check for mistakes.
Comment on lines +348 to +353
if !response.success {
if let Some(msg) = response.error_info {
return Err(Error::other(msg));
}
return Err(Error::other(""));
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns an empty error message when success=false and error_info is None. That makes debugging and log correlation difficult. Prefer returning a non-empty fallback message that includes the RPC name/parameters (e.g., 'get_live_events failed without error_info') so failures are actionable.

Copilot uses AI. Check for mistakes.
Comment on lines +39 to +49
impl ServerCertVerifier for AcceptAnyServerCertVerifier {
fn verify_server_cert(
&self,
_end_entity: &CertificateDer<'_>,
_intermediates: &[CertificateDer<'_>],
_server_name: &ServerName<'_>,
_ocsp_response: &[u8],
_now: UnixTime,
) -> Result<ServerCertVerified, RustlsError> {
Ok(ServerCertVerified::assertion())
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The E2E FTPS test disables server cert validation entirely. Even though this is test-only, it can mask real TLS issues and allows unexpected endpoints to be trusted if the test environment changes (e.g., proxying). Prefer validating against the generated self-signed certificate (pin the cert/fingerprint) or at least restricting acceptance to the expected server name/address + the generated cert.

Copilot uses AI. Check for mistakes.
…ation headers

Complete bucket notification when prefix delete returns empty ObjectInfo so webhooks match S3 DELETE behavior. Run delete-notification work via spawn_background and export the helper from storage.

Treat replication request headers like del_opts: only true/1 counts, and honor x-minio-source-replication-request. Add a regression test for header semantics.

Made-with: Cursor
EventArgs.req_params mirrors extract_params_header; many browser/console clients send x-minio-source-replication-request for compatibility. The legacy check only considered x-rustfs-source-replication-request, so honoring minio here suppressed delete webhooks for normal UI deletes.

Keep strict true/1 semantics for the rustfs header only; storage still honors both prefixes via get_header on the raw HeaderMap.

Made-with: Cursor
@andybrown668 andybrown668 force-pushed the fix-missing-delete-webhook-notification branch from e64502b to 082b35c Compare April 3, 2026 12:38
Copilot AI review requested due to automatic review settings April 3, 2026 12:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 105 out of 299 changed files in this pull request and generated 7 comments.

Comment on lines +67 to +71
if use_zero_copy {
// Try zero-copy read first (uses mmap on Unix)
let start = Instant::now();
match disk.read_file_zero_copy(bucket, path, offset, length).await {
Ok(bytes) => {
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The length variable at this point is the total end position after checksum overhead is added, while read_file_zero_copy(volume, path, offset, length) expects length to be the number of bytes to read (consistent with read_file_stream(volume, path, offset, length)). This will over-read (and may fail with FileCorrupt if offset + length exceeds file size) and differs from the previous behavior (length - offset). Fix by passing length.saturating_sub(offset) (or computing a read_len = total_len - offset) to all disk read calls here (zero-copy and stream fallback).

Copilot uses AI. Check for mistakes.
Comment on lines +107 to +111
match disk.read_file_stream(bucket, path, offset, length).await {
Ok(rd) => {
let reader = BitrotReader::new(rd, shard_size, checksum_algo, skip_verify);
Ok(Some(reader))
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as the zero-copy path: read_file_stream(bucket, path, offset, length) is passed the computed length (which is effectively an end offset), not the number of bytes to read from offset. This likely causes over-reads and breaks parity with the earlier length - offset behavior. Use a computed read length (e.g., total_len - offset) consistently.

Copilot uses AI. Check for mistakes.
Comment on lines +87 to +96
pub fn build_transition_put_options(storage_class: String, mut metadata: HashMap<String, String>) -> PutObjectOptions {
let mut opts = PutObjectOptions {
storage_class,
legalhold: ObjectLockLegalHoldStatus::from_static(""),
internal: AdvancedPutOptions {
replication_status: ReplicationStatus::from_static(""),
..Default::default()
},
..Default::default()
};
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_transition_put_options removes x-amz-replication-status from user_metadata later, but never promotes it into opts.internal.replication_status (it is always initialized to an empty value). If callers previously relied on preserving replication status across transition PUTs, this change will silently drop that data. If the intended behavior is to preserve it, parse X_AMZ_REPLICATION_STATUS from metadata and set opts.internal.replication_status before removing the key.

Copilot uses AI. Check for mistakes.
Comment on lines +88 to +95
static EVENT_DISPATCH_HOOK: OnceLock<EventDispatchHook> = OnceLock::new();

pub fn register_event_dispatch_hook<F>(hook: F) -> bool
where
F: Fn(EventArgs) + Send + Sync + 'static,
{
EVENT_DISPATCH_HOOK.set(Arc::new(hook)).is_ok()
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a global OnceLock makes hook registration one-shot for the whole process. The included unit test ignores the boolean return value, so it can become order-dependent/flaky if any other test registers the hook earlier (registration would fail and send_event would not dispatch). In the test, assert that registration succeeded (and skip/adjust behavior if it didn’t), or provide a test-only reset mechanism / alternative injection strategy that doesn’t rely on a global singleton.

Copilot uses AI. Check for mistakes.
Comment on lines +1067 to +1071
// Read all data into Bytes (single allocation)
let mut buffer = Vec::with_capacity(length);
reader.read_to_end(&mut buffer).await?;

Ok(Bytes::from(buffer))
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read_to_end is unbounded: if the remote endpoint returns more than length, this will read and allocate beyond the requested size. Since the API contract includes length, prefer enforcing the bound (e.g., wrapping the reader with take(length as u64) or pre-sizing and read_exact). This avoids accidental over-reads and reduces memory blowups under server/proxy misbehavior.

Copilot uses AI. Check for mistakes.
pub async fn save(&self) -> std::result::Result<(), std::io::Error> {
let Some(api) = new_object_layer_fn() else {
return Err(std::io::Error::other("errServerNotInitialized"));
return Err(tier_config_not_initialized_error("save tiering config"));
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the not-initialized error string away from values like errServerNotInitialized / ServerNotInitialized, while elsewhere is_err_not_initialized() relies on those substrings to classify errors. If callers depend on that classification, this becomes a behavioral regression. Consider including the established identifier in the message (or switching to a structured error variant) so programmatic detection remains consistent.

Suggested change
return Err(tier_config_not_initialized_error("save tiering config"));
return Err(std::io::Error::other(
"errServerNotInitialized: save tiering config",
));

Copilot uses AI. Check for mistakes.
}

fn tier_config_not_initialized_error(operation: &str) -> std::io::Error {
std::io::Error::other(format!("failed to {operation}: object layer not initialized"))
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the not-initialized error string away from values like errServerNotInitialized / ServerNotInitialized, while elsewhere is_err_not_initialized() relies on those substrings to classify errors. If callers depend on that classification, this becomes a behavioral regression. Consider including the established identifier in the message (or switching to a structured error variant) so programmatic detection remains consistent.

Suggested change
std::io::Error::other(format!("failed to {operation}: object layer not initialized"))
std::io::Error::other(format!(
"failed to {operation}: ServerNotInitialized: object layer not initialized"
))

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.