Fix/getobjectlength (#920)

* fix getobject content length resp

* Fix regression in exception handling for non-existent key with enhanced compression predicate and metadata improvements (#915)

* Initial plan

* Fix GetObject regression by excluding error responses from compression

The issue was that CompressionLayer was attempting to compress error responses,
which could cause Content-Length header mismatches. By excluding 4xx and 5xx
responses from compression, we ensure error responses (like NoSuchKey) are sent
correctly without body truncation.

Co-authored-by: houseme <4829346+houseme@users.noreply.github.com>

* Enhance NoSuchKey fix with improved compression predicate and comprehensive tests

- Enhanced ShouldCompress predicate with size-based exclusion (< 256 bytes)
- Added detailed documentation explaining the compression logic
- Added debug logging for better observability
- Created comprehensive test suite with 4 test cases:
  - test_get_deleted_object_returns_nosuchkey
  - test_head_deleted_object_returns_nosuchkey
  - test_get_nonexistent_object_returns_nosuchkey
  - test_multiple_gets_deleted_object
- Added extensive inline documentation and comments
- Created docs/fix-nosuchkey-regression.md with full analysis

Co-authored-by: houseme <4829346+houseme@users.noreply.github.com>

* Add compression best practices documentation

Added comprehensive guide covering:
- Best practices for HTTP response compression
- Common pitfalls and solutions
- Performance considerations and trade-offs
- Testing guidelines and examples
- Monitoring and alerting recommendations
- Migration guide for existing services

Co-authored-by: houseme <4829346+houseme@users.noreply.github.com>

* fix

* fmt

* fmt

* Fix/objectdelete (#917)

* fix getobject content length resp

* fix delete object

---------

Co-authored-by: houseme <housemecn@gmail.com>

* Add comprehensive analysis of NoSuchKey fix and related improvements

Created detailed documentation analyzing:
- HTTP compression layer fix (primary issue)
- Content-length calculation fix from PR #917
- Delete object metadata fixes from PR #917
- How all components work together
- Complete scenario walkthrough
- Performance impact analysis
- Testing strategy and deployment checklist

This ties together all the changes in the PR branch including the merged
improvements from PR #917.

Co-authored-by: houseme <4829346+houseme@users.noreply.github.com>

* replace `once_cell` to `std`

* fmt

---------

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: houseme <housemecn@gmail.com>
Co-authored-by: weisd <im@weisd.in>

* fmt

---------

Co-authored-by: weisd <weishidavip@163.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: houseme <4829346+houseme@users.noreply.github.com>
Co-authored-by: weisd <im@weisd.in>
This commit is contained in:
houseme
2025-11-24 18:56:34 +08:00
committed by GitHub
parent fce4e64da4
commit 069194f553
14 changed files with 1229 additions and 66 deletions
@@ -0,0 +1,284 @@
// 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.
//! Test for GetObject on deleted objects
//!
//! This test reproduces the issue where getting a deleted object returns
//! a networking error instead of NoSuchKey.
#![cfg(test)]
use aws_config::meta::region::RegionProviderChain;
use aws_sdk_s3::Client;
use aws_sdk_s3::config::{Credentials, Region};
use aws_sdk_s3::error::SdkError;
use bytes::Bytes;
use serial_test::serial;
use std::error::Error;
use tracing::info;
const ENDPOINT: &str = "http://localhost:9000";
const ACCESS_KEY: &str = "rustfsadmin";
const SECRET_KEY: &str = "rustfsadmin";
const BUCKET: &str = "test-get-deleted-bucket";
async fn create_aws_s3_client() -> Result<Client, Box<dyn Error>> {
let region_provider = RegionProviderChain::default_provider().or_else(Region::new("us-east-1"));
let shared_config = aws_config::defaults(aws_config::BehaviorVersion::latest())
.region(region_provider)
.credentials_provider(Credentials::new(ACCESS_KEY, SECRET_KEY, None, None, "static"))
.endpoint_url(ENDPOINT)
.load()
.await;
let client = Client::from_conf(
aws_sdk_s3::Config::from(&shared_config)
.to_builder()
.force_path_style(true)
.build(),
);
Ok(client)
}
/// Setup test bucket, creating it if it doesn't exist
async fn setup_test_bucket(client: &Client) -> Result<(), Box<dyn Error>> {
match client.create_bucket().bucket(BUCKET).send().await {
Ok(_) => {}
Err(SdkError::ServiceError(e)) => {
let e = e.into_err();
let error_code = e.meta().code().unwrap_or("");
if !error_code.eq("BucketAlreadyExists") && !error_code.eq("BucketAlreadyOwnedByYou") {
return Err(e.into());
}
}
Err(e) => {
return Err(e.into());
}
}
Ok(())
}
#[tokio::test]
#[serial]
#[ignore = "requires running RustFS server at localhost:9000"]
async fn test_get_deleted_object_returns_nosuchkey() -> Result<(), Box<dyn std::error::Error>> {
// Initialize logging
let _ = tracing_subscriber::fmt()
.with_max_level(tracing::Level::INFO)
.with_test_writer()
.try_init();
info!("🧪 Starting test_get_deleted_object_returns_nosuchkey");
let client = create_aws_s3_client().await?;
setup_test_bucket(&client).await?;
// Upload a test object
let key = "test-file-to-delete.txt";
let content = b"This will be deleted soon!";
info!("Uploading object: {}", key);
client
.put_object()
.bucket(BUCKET)
.key(key)
.body(Bytes::from_static(content).into())
.send()
.await?;
// Verify object exists
info!("Verifying object exists");
let get_result = client.get_object().bucket(BUCKET).key(key).send().await;
assert!(get_result.is_ok(), "Object should exist after upload");
// Delete the object
info!("Deleting object: {}", key);
client.delete_object().bucket(BUCKET).key(key).send().await?;
// Try to get the deleted object - should return NoSuchKey error
info!("Attempting to get deleted object - expecting NoSuchKey error");
let get_result = client.get_object().bucket(BUCKET).key(key).send().await;
// Check that we get an error
assert!(get_result.is_err(), "Getting deleted object should return an error");
// Check that the error is NoSuchKey, not a networking error
let err = get_result.unwrap_err();
// Print the error for debugging
info!("Error received: {:?}", err);
// Check if it's a service error
match err {
SdkError::ServiceError(service_err) => {
let s3_err = service_err.into_err();
info!("Service error code: {:?}", s3_err.meta().code());
// The error should be NoSuchKey
assert!(s3_err.is_no_such_key(), "Error should be NoSuchKey, got: {:?}", s3_err);
info!("✅ Test passed: GetObject on deleted object correctly returns NoSuchKey");
}
other_err => {
panic!("Expected ServiceError with NoSuchKey, but got: {:?}", other_err);
}
}
// Cleanup
let _ = client.delete_object().bucket(BUCKET).key(key).send().await;
Ok(())
}
/// Test that HeadObject on a deleted object also returns NoSuchKey
#[tokio::test]
#[serial]
#[ignore = "requires running RustFS server at localhost:9000"]
async fn test_head_deleted_object_returns_nosuchkey() -> Result<(), Box<dyn std::error::Error>> {
let _ = tracing_subscriber::fmt()
.with_max_level(tracing::Level::INFO)
.with_test_writer()
.try_init();
info!("🧪 Starting test_head_deleted_object_returns_nosuchkey");
let client = create_aws_s3_client().await?;
setup_test_bucket(&client).await?;
let key = "test-head-deleted.txt";
let content = b"Test content for HeadObject";
// Upload and verify
client
.put_object()
.bucket(BUCKET)
.key(key)
.body(Bytes::from_static(content).into())
.send()
.await?;
// Delete the object
client.delete_object().bucket(BUCKET).key(key).send().await?;
// Try to head the deleted object
let head_result = client.head_object().bucket(BUCKET).key(key).send().await;
assert!(head_result.is_err(), "HeadObject on deleted object should return an error");
match head_result.unwrap_err() {
SdkError::ServiceError(service_err) => {
let s3_err = service_err.into_err();
assert!(
s3_err.meta().code() == Some("NoSuchKey") || s3_err.meta().code() == Some("NotFound"),
"Error should be NoSuchKey or NotFound, got: {:?}",
s3_err
);
info!("✅ HeadObject correctly returns NoSuchKey/NotFound");
}
other_err => {
panic!("Expected ServiceError but got: {:?}", other_err);
}
}
Ok(())
}
/// Test GetObject with non-existent key (never existed)
#[tokio::test]
#[serial]
#[ignore = "requires running RustFS server at localhost:9000"]
async fn test_get_nonexistent_object_returns_nosuchkey() -> Result<(), Box<dyn std::error::Error>> {
let _ = tracing_subscriber::fmt()
.with_max_level(tracing::Level::INFO)
.with_test_writer()
.try_init();
info!("🧪 Starting test_get_nonexistent_object_returns_nosuchkey");
let client = create_aws_s3_client().await?;
setup_test_bucket(&client).await?;
// Try to get an object that never existed
let key = "this-key-never-existed.txt";
let get_result = client.get_object().bucket(BUCKET).key(key).send().await;
assert!(get_result.is_err(), "Getting non-existent object should return an error");
match get_result.unwrap_err() {
SdkError::ServiceError(service_err) => {
let s3_err = service_err.into_err();
assert!(s3_err.is_no_such_key(), "Error should be NoSuchKey, got: {:?}", s3_err);
info!("✅ GetObject correctly returns NoSuchKey for non-existent object");
}
other_err => {
panic!("Expected ServiceError with NoSuchKey, but got: {:?}", other_err);
}
}
Ok(())
}
/// Test multiple consecutive GetObject calls on deleted object
/// This ensures the fix is stable and doesn't have race conditions
#[tokio::test]
#[serial]
#[ignore = "requires running RustFS server at localhost:9000"]
async fn test_multiple_gets_deleted_object() -> Result<(), Box<dyn std::error::Error>> {
let _ = tracing_subscriber::fmt()
.with_max_level(tracing::Level::INFO)
.with_test_writer()
.try_init();
info!("🧪 Starting test_multiple_gets_deleted_object");
let client = create_aws_s3_client().await?;
setup_test_bucket(&client).await?;
let key = "test-multiple-gets.txt";
let content = b"Test content";
// Upload and delete
client
.put_object()
.bucket(BUCKET)
.key(key)
.body(Bytes::from_static(content).into())
.send()
.await?;
client.delete_object().bucket(BUCKET).key(key).send().await?;
// Try multiple consecutive GetObject calls
for i in 1..=5 {
info!("Attempt {} to get deleted object", i);
let get_result = client.get_object().bucket(BUCKET).key(key).send().await;
assert!(get_result.is_err(), "Attempt {}: should return error", i);
match get_result.unwrap_err() {
SdkError::ServiceError(service_err) => {
let s3_err = service_err.into_err();
assert!(s3_err.is_no_such_key(), "Attempt {}: Error should be NoSuchKey, got: {:?}", i, s3_err);
}
other_err => {
panic!("Attempt {}: Expected ServiceError but got: {:?}", i, other_err);
}
}
}
info!("✅ All 5 attempts correctly returned NoSuchKey");
Ok(())
}
+1
View File
@@ -13,6 +13,7 @@
// limitations under the License.
mod conditional_writes;
mod get_deleted_object_test;
mod lifecycle;
mod lock;
mod node_interact_test;
-1
View File
@@ -41,7 +41,6 @@ tracing.workspace = true
url.workspace = true
uuid.workspace = true
thiserror.workspace = true
once_cell.workspace = true
parking_lot.workspace = true
smallvec.workspace = true
smartstring.workspace = true
@@ -12,14 +12,14 @@
// See the License for the specific language governing permissions and
// limitations under the License.
use once_cell::sync::Lazy;
use std::sync::Arc;
use std::sync::LazyLock;
use std::sync::atomic::{AtomicU32, AtomicUsize, Ordering};
use tokio::sync::Notify;
/// Optimized notification pool to reduce memory overhead and thundering herd effects
/// Increased pool size for better performance under high concurrency
static NOTIFY_POOL: Lazy<Vec<Arc<Notify>>> = Lazy::new(|| (0..128).map(|_| Arc::new(Notify::new())).collect());
static NOTIFY_POOL: LazyLock<Vec<Arc<Notify>>> = LazyLock::new(|| (0..128).map(|_| Arc::new(Notify::new())).collect());
/// Optimized notification system for object locks
#[derive(Debug)]
+7 -7
View File
@@ -12,11 +12,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.
use once_cell::unsync::OnceCell;
use serde::{Deserialize, Serialize};
use smartstring::SmartString;
use std::hash::{Hash, Hasher};
use std::sync::Arc;
use std::sync::OnceLock;
use std::time::{Duration, SystemTime};
use crate::fast_lock::guard::FastLockGuard;
@@ -72,10 +72,10 @@ pub struct OptimizedObjectKey {
/// Version - optional for latest version semantics
pub version: Option<SmartString<smartstring::LazyCompact>>,
/// Cached hash to avoid recomputation
hash_cache: OnceCell<u64>,
hash_cache: OnceLock<u64>,
}
// Manual implementations to handle OnceCell properly
// Manual implementations to handle OnceLock properly
impl PartialEq for OptimizedObjectKey {
fn eq(&self, other: &Self) -> bool {
self.bucket == other.bucket && self.object == other.object && self.version == other.version
@@ -116,7 +116,7 @@ impl OptimizedObjectKey {
bucket: bucket.into(),
object: object.into(),
version: None,
hash_cache: OnceCell::new(),
hash_cache: OnceLock::new(),
}
}
@@ -129,7 +129,7 @@ impl OptimizedObjectKey {
bucket: bucket.into(),
object: object.into(),
version: Some(version.into()),
hash_cache: OnceCell::new(),
hash_cache: OnceLock::new(),
}
}
@@ -145,7 +145,7 @@ impl OptimizedObjectKey {
/// Reset hash cache if key is modified
pub fn invalidate_cache(&mut self) {
self.hash_cache = OnceCell::new();
self.hash_cache = OnceLock::new();
}
/// Convert from regular ObjectKey
@@ -154,7 +154,7 @@ impl OptimizedObjectKey {
bucket: SmartString::from(key.bucket.as_ref()),
object: SmartString::from(key.object.as_ref()),
version: key.version.as_ref().map(|v| SmartString::from(v.as_ref())),
hash_cache: OnceCell::new(),
hash_cache: OnceLock::new(),
}
}
+3 -6
View File
@@ -12,12 +12,9 @@
// See the License for the specific language governing permissions and
// limitations under the License.
use std::sync::Arc;
use once_cell::sync::Lazy;
use tokio::sync::mpsc;
use crate::{client::LockClient, types::LockId};
use std::sync::{Arc, LazyLock};
use tokio::sync::mpsc;
#[derive(Debug, Clone)]
struct UnlockJob {
@@ -31,7 +28,7 @@ struct UnlockRuntime {
}
// Global unlock runtime with background worker
static UNLOCK_RUNTIME: Lazy<UnlockRuntime> = Lazy::new(|| {
static UNLOCK_RUNTIME: LazyLock<UnlockRuntime> = LazyLock::new(|| {
// Larger buffer to reduce contention during bursts
let (tx, mut rx) = mpsc::channel::<UnlockJob>(8192);
+21 -24
View File
@@ -73,13 +73,13 @@ pub const MAX_DELETE_LIST: usize = 1000;
// ============================================================================
// Global singleton FastLock manager shared across all lock implementations
use once_cell::sync::OnceCell;
use std::sync::Arc;
use std::sync::OnceLock;
/// Enum wrapper for different lock manager implementations
pub enum GlobalLockManager {
Enabled(Arc<fast_lock::FastObjectLockManager>),
Disabled(fast_lock::DisabledLockManager),
Enabled(Arc<FastObjectLockManager>),
Disabled(DisabledLockManager),
}
impl Default for GlobalLockManager {
@@ -99,11 +99,11 @@ impl GlobalLockManager {
match locks_enabled.as_str() {
"false" | "0" | "no" | "off" | "disabled" => {
tracing::info!("Lock system disabled via RUSTFS_ENABLE_LOCKS environment variable");
Self::Disabled(fast_lock::DisabledLockManager::new())
Self::Disabled(DisabledLockManager::new())
}
_ => {
tracing::info!("Lock system enabled");
Self::Enabled(Arc::new(fast_lock::FastObjectLockManager::new()))
Self::Enabled(Arc::new(FastObjectLockManager::new()))
}
}
}
@@ -114,7 +114,7 @@ impl GlobalLockManager {
}
/// Get the FastObjectLockManager if enabled, otherwise returns None
pub fn as_fast_lock_manager(&self) -> Option<Arc<fast_lock::FastObjectLockManager>> {
pub fn as_fast_lock_manager(&self) -> Option<Arc<FastObjectLockManager>> {
match self {
Self::Enabled(manager) => Some(manager.clone()),
Self::Disabled(_) => None,
@@ -123,11 +123,8 @@ impl GlobalLockManager {
}
#[async_trait::async_trait]
impl fast_lock::LockManager for GlobalLockManager {
async fn acquire_lock(
&self,
request: fast_lock::ObjectLockRequest,
) -> std::result::Result<fast_lock::FastLockGuard, fast_lock::LockResult> {
impl LockManager for GlobalLockManager {
async fn acquire_lock(&self, request: ObjectLockRequest) -> std::result::Result<FastLockGuard, LockResult> {
match self {
Self::Enabled(manager) => manager.acquire_lock(request).await,
Self::Disabled(manager) => manager.acquire_lock(request).await,
@@ -139,7 +136,7 @@ impl fast_lock::LockManager for GlobalLockManager {
bucket: impl Into<Arc<str>> + Send,
object: impl Into<Arc<str>> + Send,
owner: impl Into<Arc<str>> + Send,
) -> std::result::Result<fast_lock::FastLockGuard, fast_lock::LockResult> {
) -> std::result::Result<FastLockGuard, LockResult> {
match self {
Self::Enabled(manager) => manager.acquire_read_lock(bucket, object, owner).await,
Self::Disabled(manager) => manager.acquire_read_lock(bucket, object, owner).await,
@@ -152,7 +149,7 @@ impl fast_lock::LockManager for GlobalLockManager {
object: impl Into<Arc<str>> + Send,
version: impl Into<Arc<str>> + Send,
owner: impl Into<Arc<str>> + Send,
) -> std::result::Result<fast_lock::FastLockGuard, fast_lock::LockResult> {
) -> std::result::Result<FastLockGuard, LockResult> {
match self {
Self::Enabled(manager) => manager.acquire_read_lock_versioned(bucket, object, version, owner).await,
Self::Disabled(manager) => manager.acquire_read_lock_versioned(bucket, object, version, owner).await,
@@ -164,7 +161,7 @@ impl fast_lock::LockManager for GlobalLockManager {
bucket: impl Into<Arc<str>> + Send,
object: impl Into<Arc<str>> + Send,
owner: impl Into<Arc<str>> + Send,
) -> std::result::Result<fast_lock::FastLockGuard, fast_lock::LockResult> {
) -> std::result::Result<FastLockGuard, LockResult> {
match self {
Self::Enabled(manager) => manager.acquire_write_lock(bucket, object, owner).await,
Self::Disabled(manager) => manager.acquire_write_lock(bucket, object, owner).await,
@@ -177,21 +174,21 @@ impl fast_lock::LockManager for GlobalLockManager {
object: impl Into<Arc<str>> + Send,
version: impl Into<Arc<str>> + Send,
owner: impl Into<Arc<str>> + Send,
) -> std::result::Result<fast_lock::FastLockGuard, fast_lock::LockResult> {
) -> std::result::Result<FastLockGuard, LockResult> {
match self {
Self::Enabled(manager) => manager.acquire_write_lock_versioned(bucket, object, version, owner).await,
Self::Disabled(manager) => manager.acquire_write_lock_versioned(bucket, object, version, owner).await,
}
}
async fn acquire_locks_batch(&self, batch_request: fast_lock::BatchLockRequest) -> fast_lock::BatchLockResult {
async fn acquire_locks_batch(&self, batch_request: BatchLockRequest) -> BatchLockResult {
match self {
Self::Enabled(manager) => manager.acquire_locks_batch(batch_request).await,
Self::Disabled(manager) => manager.acquire_locks_batch(batch_request).await,
}
}
fn get_lock_info(&self, key: &fast_lock::ObjectKey) -> Option<fast_lock::ObjectLockInfo> {
fn get_lock_info(&self, key: &ObjectKey) -> Option<ObjectLockInfo> {
match self {
Self::Enabled(manager) => manager.get_lock_info(key),
Self::Disabled(manager) => manager.get_lock_info(key),
@@ -248,7 +245,7 @@ impl fast_lock::LockManager for GlobalLockManager {
}
}
static GLOBAL_LOCK_MANAGER: OnceCell<Arc<GlobalLockManager>> = OnceCell::new();
static GLOBAL_LOCK_MANAGER: OnceLock<Arc<GlobalLockManager>> = OnceLock::new();
/// Get the global shared lock manager instance
///
@@ -263,7 +260,7 @@ pub fn get_global_lock_manager() -> Arc<GlobalLockManager> {
/// This function is deprecated. Use get_global_lock_manager() instead.
/// Returns FastObjectLockManager when locks are enabled, or panics when disabled.
#[deprecated(note = "Use get_global_lock_manager() instead")]
pub fn get_global_fast_lock_manager() -> Arc<fast_lock::FastObjectLockManager> {
pub fn get_global_fast_lock_manager() -> Arc<FastObjectLockManager> {
let manager = get_global_lock_manager();
manager.as_fast_lock_manager().unwrap_or_else(|| {
panic!("Cannot get FastObjectLockManager when locks are disabled. Use get_global_lock_manager() instead.");
@@ -301,7 +298,7 @@ mod tests {
#[tokio::test]
async fn test_disabled_manager_direct() {
let manager = fast_lock::DisabledLockManager::new();
let manager = DisabledLockManager::new();
// All operations should succeed immediately
let guard = manager.acquire_read_lock("bucket", "object", "owner").await;
@@ -316,7 +313,7 @@ mod tests {
#[tokio::test]
async fn test_enabled_manager_direct() {
let manager = fast_lock::FastObjectLockManager::new();
let manager = FastObjectLockManager::new();
// Operations should work normally
let guard = manager.acquire_read_lock("bucket", "object", "owner").await;
@@ -331,8 +328,8 @@ mod tests {
#[tokio::test]
async fn test_global_manager_enum_wrapper() {
// Test the GlobalLockManager enum directly
let enabled_manager = GlobalLockManager::Enabled(Arc::new(fast_lock::FastObjectLockManager::new()));
let disabled_manager = GlobalLockManager::Disabled(fast_lock::DisabledLockManager::new());
let enabled_manager = GlobalLockManager::Enabled(Arc::new(FastObjectLockManager::new()));
let disabled_manager = GlobalLockManager::Disabled(DisabledLockManager::new());
assert!(!enabled_manager.is_disabled());
assert!(disabled_manager.is_disabled());
@@ -352,7 +349,7 @@ mod tests {
async fn test_batch_operations_work() {
let manager = get_global_lock_manager();
let batch = fast_lock::BatchLockRequest::new("owner")
let batch = BatchLockRequest::new("owner")
.add_read_lock("bucket", "obj1")
.add_write_lock("bucket", "obj2");