From 39fc461597a0e98e5bd25a14ca86b3136e3bf2a9 Mon Sep 17 00:00:00 2001 From: Yixuan Wang Date: Thu, 30 Apr 2026 11:20:55 +0800 Subject: [PATCH 1/4] [fix](cloud) Check object existence before file delete ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: S3-compatible object deletes can succeed even when the target object does not exist, so recycler file deletion could silently ignore missing files. Check object existence in the object storage client before explicit file deletion and propagate NOT_FOUND to the accessor. ### Release note None ### Check List (For Author) - Test: No need to test (requested to only change code and not write or run tests) - Behavior changed: Yes (explicit file deletion now returns an error when the object is missing) - Does this need documentation: No --- cloud/src/recycler/azure_obj_client.cpp | 26 ++++++++++++++++++++++--- cloud/src/recycler/azure_obj_client.h | 2 ++ cloud/src/recycler/obj_storage_client.h | 1 + cloud/src/recycler/s3_accessor.cpp | 6 ++++-- cloud/src/recycler/s3_obj_client.cpp | 22 ++++++++++++++++++++- cloud/src/recycler/s3_obj_client.h | 4 +++- 6 files changed, 54 insertions(+), 7 deletions(-) diff --git a/cloud/src/recycler/azure_obj_client.cpp b/cloud/src/recycler/azure_obj_client.cpp index 65c0a45240353e..f03f5b823964ac 100644 --- a/cloud/src/recycler/azure_obj_client.cpp +++ b/cloud/src/recycler/azure_obj_client.cpp @@ -234,12 +234,22 @@ std::unique_ptr AzureObjClient::list_objects(ObjectStoragePa // You can find out the num in https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch?tabs=microsoft-entra-id // > Each batch request supports a maximum of 256 subrequests. ObjectStorageResponse AzureObjClient::delete_objects(const std::string& bucket, - std::vector keys, - ObjClientOptions option) { + std::vector keys, + ObjClientOptions option) { if (keys.empty()) { return {0}; } + if (option.check_exists_before_delete) { + ObjectMeta obj_meta; + for (const auto& key : keys) { + auto head_resp = head_object({.bucket = bucket, .key = key}, &obj_meta); + if (head_resp.ret != ObjectStorageResponse::OK) { + return head_resp; + } + } + } + // TODO(ByteYue) : use range to adate this code when compiler is ready // auto chunkedView = objs | std::views::chunk(BlobBatchMaxOperations); auto begin = std::begin(keys); @@ -295,6 +305,16 @@ ObjectStorageResponse AzureObjClient::delete_objects(const std::string& bucket, } ObjectStorageResponse AzureObjClient::delete_object(ObjectStoragePathRef path) { + ObjectMeta obj_meta; + auto head_resp = head_object(path, &obj_meta); + if (head_resp.ret != ObjectStorageResponse::OK) { + return head_resp; + } + + return delete_object_impl(path); +} + +ObjectStorageResponse AzureObjClient::delete_object_impl(ObjectStoragePathRef path) { return do_azure_client_call( [&]() { if (auto r = s3_put_rate_limit([&]() { @@ -333,4 +353,4 @@ ObjectStorageResponse AzureObjClient::abort_multipart_upload(ObjectStoragePathRe return delete_object(path); } -} // namespace doris::cloud \ No newline at end of file +} // namespace doris::cloud diff --git a/cloud/src/recycler/azure_obj_client.h b/cloud/src/recycler/azure_obj_client.h index bdf9b4eda752cc..8660704db8651e 100644 --- a/cloud/src/recycler/azure_obj_client.h +++ b/cloud/src/recycler/azure_obj_client.h @@ -56,6 +56,8 @@ class AzureObjClient final : public ObjStorageClient { const std::string& upload_id) override; private: + ObjectStorageResponse delete_object_impl(ObjectStoragePathRef path); + std::shared_ptr client_; }; diff --git a/cloud/src/recycler/obj_storage_client.h b/cloud/src/recycler/obj_storage_client.h index 358dfba4943fd7..d46b0fbdbfa103 100644 --- a/cloud/src/recycler/obj_storage_client.h +++ b/cloud/src/recycler/obj_storage_client.h @@ -60,6 +60,7 @@ class ObjectListIterator { class SimpleThreadPool; struct ObjClientOptions { bool prefetch {true}; + bool check_exists_before_delete {false}; std::shared_ptr executor; }; diff --git a/cloud/src/recycler/s3_accessor.cpp b/cloud/src/recycler/s3_accessor.cpp index 0f2a7776fcc7fe..25b82d3f6b83fc 100644 --- a/cloud/src/recycler/s3_accessor.cpp +++ b/cloud/src/recycler/s3_accessor.cpp @@ -479,7 +479,9 @@ int S3Accessor::delete_files(const std::vector& paths) { keys.emplace_back(get_key(path)); } - return obj_client_->delete_objects(conf_.bucket, std::move(keys), {.executor = worker_pool}) + return obj_client_ + ->delete_objects(conf_.bucket, std::move(keys), + {.check_exists_before_delete = true, .executor = worker_pool}) .ret; } @@ -487,7 +489,7 @@ int S3Accessor::delete_file(const std::string& path) { LOG_INFO("delete file").tag("uri", to_uri(path)); int ret = obj_client_->delete_object({.bucket = conf_.bucket, .key = get_key(path)}).ret; static_assert(ObjectStorageResponse::OK == 0); - if (ret == ObjectStorageResponse::OK || ret == ObjectStorageResponse::NOT_FOUND) { + if (ret == ObjectStorageResponse::OK) { return 0; } return ret; diff --git a/cloud/src/recycler/s3_obj_client.cpp b/cloud/src/recycler/s3_obj_client.cpp index 81d791cabac465..4341cbb7722901 100644 --- a/cloud/src/recycler/s3_obj_client.cpp +++ b/cloud/src/recycler/s3_obj_client.cpp @@ -259,13 +259,23 @@ ObjectStorageResponse S3ObjClient::delete_objects(const std::string& bucket, return {0}; } + if (option.check_exists_before_delete) { + ObjectMeta obj_meta; + for (const auto& key : keys) { + auto head_resp = head_object({.bucket = bucket, .key = key}, &obj_meta); + if (head_resp.ret != ObjectStorageResponse::OK) { + return head_resp; + } + } + } + Aws::S3::Model::DeleteObjectsRequest delete_request; delete_request.SetBucket(bucket); auto issue_delete = [&bucket, &delete_request, this](std::vector objects) -> int { if (objects.size() == 1) { - return delete_object({.bucket = bucket, .key = objects[0].GetKey()}).ret; + return delete_object_impl({.bucket = bucket, .key = objects[0].GetKey()}).ret; } Aws::S3::Model::Delete del; @@ -318,6 +328,16 @@ ObjectStorageResponse S3ObjClient::delete_objects(const std::string& bucket, } ObjectStorageResponse S3ObjClient::delete_object(ObjectStoragePathRef path) { + ObjectMeta obj_meta; + auto head_resp = head_object(path, &obj_meta); + if (head_resp.ret != ObjectStorageResponse::OK) { + return head_resp; + } + + return delete_object_impl(path); +} + +ObjectStorageResponse S3ObjClient::delete_object_impl(ObjectStoragePathRef path) { Aws::S3::Model::DeleteObjectRequest request; request.WithBucket(path.bucket).WithKey(path.key); auto outcome = s3_put_rate_limit([&]() { diff --git a/cloud/src/recycler/s3_obj_client.h b/cloud/src/recycler/s3_obj_client.h index e53564b6c9f708..bf9b90462dedc3 100644 --- a/cloud/src/recycler/s3_obj_client.h +++ b/cloud/src/recycler/s3_obj_client.h @@ -57,8 +57,10 @@ class S3ObjClient final : public ObjStorageClient { const std::string& upload_id) override; private: + ObjectStorageResponse delete_object_impl(ObjectStoragePathRef path); + std::shared_ptr s3_client_; std::string endpoint_; }; -} // namespace doris::cloud \ No newline at end of file +} // namespace doris::cloud From 10f2c783b0b5602f74c07709de664ed5751859bb Mon Sep 17 00:00:00 2001 From: Yixuan Wang Date: Thu, 30 Apr 2026 11:23:04 +0800 Subject: [PATCH 2/4] [improvement](cloud) Add config for delete existence check ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: The object existence check before explicit file deletion should be configurable because it adds an extra HEAD request. Add a default-on cloud config and keep the strict delete behavior enabled by default. ### Release note None ### Check List (For Author) - Test: No need to test (requested to only change code and not write or run tests) - Behavior changed: No (default behavior remains checking object existence before explicit file deletion) - Does this need documentation: No --- cloud/src/common/config.h | 2 ++ cloud/src/recycler/azure_obj_client.cpp | 16 ++++++++++++---- cloud/src/recycler/s3_accessor.cpp | 4 +++- cloud/src/recycler/s3_obj_client.cpp | 16 ++++++++++++---- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/cloud/src/common/config.h b/cloud/src/common/config.h index 770ee9ca13a4f5..07bde01f8b9092 100644 --- a/cloud/src/common/config.h +++ b/cloud/src/common/config.h @@ -104,6 +104,8 @@ CONF_mInt32(instance_recycler_worker_pool_size, "32"); // Max number of delete tasks per batch when recycling objects. // Each task deletes up to 1000 files. Controls memory usage during large-scale deletion. CONF_Int32(recycler_max_tasks_per_batch, "1000"); +// Check object existence before deleting explicit files from object storage. +CONF_mBool(enable_delete_file_check_object_exists, "true"); // The worker pool size for http api `statistics_recycle` worker pool CONF_mInt32(instance_recycler_statistics_recycle_worker_pool_size, "5"); CONF_Bool(enable_checker, "false"); diff --git a/cloud/src/recycler/azure_obj_client.cpp b/cloud/src/recycler/azure_obj_client.cpp index f03f5b823964ac..611ca10c34a5a7 100644 --- a/cloud/src/recycler/azure_obj_client.cpp +++ b/cloud/src/recycler/azure_obj_client.cpp @@ -244,6 +244,9 @@ ObjectStorageResponse AzureObjClient::delete_objects(const std::string& bucket, ObjectMeta obj_meta; for (const auto& key : keys) { auto head_resp = head_object({.bucket = bucket, .key = key}, &obj_meta); + DCHECK_EQ(head_resp.ret, ObjectStorageResponse::OK) + << "bucket=" << bucket << " key=" << key + << " error=" << head_resp.error_msg; if (head_resp.ret != ObjectStorageResponse::OK) { return head_resp; } @@ -305,10 +308,15 @@ ObjectStorageResponse AzureObjClient::delete_objects(const std::string& bucket, } ObjectStorageResponse AzureObjClient::delete_object(ObjectStoragePathRef path) { - ObjectMeta obj_meta; - auto head_resp = head_object(path, &obj_meta); - if (head_resp.ret != ObjectStorageResponse::OK) { - return head_resp; + if (config::enable_delete_file_check_object_exists) { + ObjectMeta obj_meta; + auto head_resp = head_object(path, &obj_meta); + DCHECK_EQ(head_resp.ret, ObjectStorageResponse::OK) + << "bucket=" << path.bucket << " key=" << path.key + << " error=" << head_resp.error_msg; + if (head_resp.ret != ObjectStorageResponse::OK) { + return head_resp; + } } return delete_object_impl(path); diff --git a/cloud/src/recycler/s3_accessor.cpp b/cloud/src/recycler/s3_accessor.cpp index 25b82d3f6b83fc..d902422e66ffbe 100644 --- a/cloud/src/recycler/s3_accessor.cpp +++ b/cloud/src/recycler/s3_accessor.cpp @@ -481,7 +481,9 @@ int S3Accessor::delete_files(const std::vector& paths) { return obj_client_ ->delete_objects(conf_.bucket, std::move(keys), - {.check_exists_before_delete = true, .executor = worker_pool}) + {.check_exists_before_delete = + config::enable_delete_file_check_object_exists, + .executor = worker_pool}) .ret; } diff --git a/cloud/src/recycler/s3_obj_client.cpp b/cloud/src/recycler/s3_obj_client.cpp index 4341cbb7722901..d8d3a2a3af4ebb 100644 --- a/cloud/src/recycler/s3_obj_client.cpp +++ b/cloud/src/recycler/s3_obj_client.cpp @@ -263,6 +263,9 @@ ObjectStorageResponse S3ObjClient::delete_objects(const std::string& bucket, ObjectMeta obj_meta; for (const auto& key : keys) { auto head_resp = head_object({.bucket = bucket, .key = key}, &obj_meta); + DCHECK_EQ(head_resp.ret, ObjectStorageResponse::OK) + << "bucket=" << bucket << " key=" << key + << " error=" << head_resp.error_msg; if (head_resp.ret != ObjectStorageResponse::OK) { return head_resp; } @@ -328,10 +331,15 @@ ObjectStorageResponse S3ObjClient::delete_objects(const std::string& bucket, } ObjectStorageResponse S3ObjClient::delete_object(ObjectStoragePathRef path) { - ObjectMeta obj_meta; - auto head_resp = head_object(path, &obj_meta); - if (head_resp.ret != ObjectStorageResponse::OK) { - return head_resp; + if (config::enable_delete_file_check_object_exists) { + ObjectMeta obj_meta; + auto head_resp = head_object(path, &obj_meta); + DCHECK_EQ(head_resp.ret, ObjectStorageResponse::OK) + << "bucket=" << path.bucket << " key=" << path.key + << " error=" << head_resp.error_msg; + if (head_resp.ret != ObjectStorageResponse::OK) { + return head_resp; + } } return delete_object_impl(path); From d52667b77733b6abe541b57070356965f52ce004 Mon Sep 17 00:00:00 2001 From: Yixuan Wang Date: Thu, 30 Apr 2026 11:29:53 +0800 Subject: [PATCH 3/4] [test](cloud) Add delete object existence check test ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: The configurable object existence check before S3 object deletion needs unit coverage to verify the HEAD request is issued when enabled and skipped when disabled. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-cloud-ut.sh --run --filter=s3_accessor_client_test:S3ObjClientTest.DeleteObjectCheckExistsConfigTest - Behavior changed: No - Does this need documentation: No --- cloud/test/s3_accessor_client_test.cpp | 70 ++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/cloud/test/s3_accessor_client_test.cpp b/cloud/test/s3_accessor_client_test.cpp index f6a8802cf22f46..6f2c36e423bea3 100644 --- a/cloud/test/s3_accessor_client_test.cpp +++ b/cloud/test/s3_accessor_client_test.cpp @@ -16,6 +16,9 @@ // under the License. #include +#include +#include +#include #include #include #include @@ -33,6 +36,7 @@ #include "cpp/sync_point.h" #include "recycler/recycler_service.h" #include "recycler/s3_accessor.h" +#include "recycler/s3_obj_client.h" using namespace doris; @@ -54,6 +58,72 @@ int main(int argc, char** argv) { namespace doris::cloud { +class MockDeleteS3Client : public Aws::S3::S3Client { +public: + Aws::S3::Model::HeadObjectOutcome HeadObject( + const Aws::S3::Model::HeadObjectRequest& request) const override { + ++head_count; + last_head_key = request.GetKey(); + if (exists) { + return Aws::S3::Model::HeadObjectOutcome(Aws::S3::Model::HeadObjectResult()); + } + + auto error = Aws::Client::AWSError( + Aws::S3::S3Errors::RESOURCE_NOT_FOUND, false); + error.SetResponseCode(Aws::Http::HttpResponseCode::NOT_FOUND); + return Aws::S3::Model::HeadObjectOutcome(error); + } + + Aws::S3::Model::DeleteObjectOutcome DeleteObject( + const Aws::S3::Model::DeleteObjectRequest& request) const override { + ++delete_count; + last_delete_key = request.GetKey(); + return Aws::S3::Model::DeleteObjectOutcome(Aws::S3::Model::DeleteObjectResult()); + } + + mutable int head_count {0}; + mutable int delete_count {0}; + mutable std::string last_head_key; + mutable std::string last_delete_key; + bool exists {true}; +}; + +class S3ObjClientTest : public testing::Test { +protected: + static void SetUpTestSuite() { Aws::InitAPI(options); } + + static void TearDownTestSuite() { Aws::ShutdownAPI(options); } + +private: + static Aws::SDKOptions options; +}; + +Aws::SDKOptions S3ObjClientTest::options {}; + +TEST_F(S3ObjClientTest, DeleteObjectCheckExistsConfigTest) { + auto mock_s3_client = std::make_shared(); + S3ObjClient obj_client(mock_s3_client, "dummy-endpoint"); + const bool original_check_exists = config::enable_delete_file_check_object_exists; + + config::enable_delete_file_check_object_exists = true; + auto response = obj_client.delete_object({.bucket = "dummy-bucket", .key = "existing-key"}); + EXPECT_EQ(response.ret, ObjectStorageResponse::OK); + EXPECT_EQ(mock_s3_client->head_count, 1); + EXPECT_EQ(mock_s3_client->delete_count, 1); + EXPECT_EQ(mock_s3_client->last_head_key, "existing-key"); + EXPECT_EQ(mock_s3_client->last_delete_key, "existing-key"); + + mock_s3_client->exists = false; + config::enable_delete_file_check_object_exists = false; + response = obj_client.delete_object({.bucket = "dummy-bucket", .key = "missing-key"}); + EXPECT_EQ(response.ret, ObjectStorageResponse::OK); + EXPECT_EQ(mock_s3_client->head_count, 1); + EXPECT_EQ(mock_s3_client->delete_count, 2); + EXPECT_EQ(mock_s3_client->last_delete_key, "missing-key"); + + config::enable_delete_file_check_object_exists = original_check_exists; +} + #define GET_ENV_IF_DEFINED(var) \ ([]() -> std::string { \ const char* val = std::getenv(#var); \ From e3cbb6f7f25abaae91561853597567c23c4a4881 Mon Sep 17 00:00:00 2001 From: Yixuan Wang Date: Thu, 30 Apr 2026 16:17:10 +0800 Subject: [PATCH 4/4] format --- cloud/src/recycler/azure_obj_client.cpp | 7 +++---- cloud/src/recycler/s3_accessor.cpp | 8 ++++---- cloud/src/recycler/s3_obj_client.cpp | 3 +-- cloud/test/s3_accessor_client_test.cpp | 6 +++--- 4 files changed, 11 insertions(+), 13 deletions(-) diff --git a/cloud/src/recycler/azure_obj_client.cpp b/cloud/src/recycler/azure_obj_client.cpp index 611ca10c34a5a7..afe02ebaa9ec16 100644 --- a/cloud/src/recycler/azure_obj_client.cpp +++ b/cloud/src/recycler/azure_obj_client.cpp @@ -234,8 +234,8 @@ std::unique_ptr AzureObjClient::list_objects(ObjectStoragePa // You can find out the num in https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch?tabs=microsoft-entra-id // > Each batch request supports a maximum of 256 subrequests. ObjectStorageResponse AzureObjClient::delete_objects(const std::string& bucket, - std::vector keys, - ObjClientOptions option) { + std::vector keys, + ObjClientOptions option) { if (keys.empty()) { return {0}; } @@ -245,8 +245,7 @@ ObjectStorageResponse AzureObjClient::delete_objects(const std::string& bucket, for (const auto& key : keys) { auto head_resp = head_object({.bucket = bucket, .key = key}, &obj_meta); DCHECK_EQ(head_resp.ret, ObjectStorageResponse::OK) - << "bucket=" << bucket << " key=" << key - << " error=" << head_resp.error_msg; + << "bucket=" << bucket << " key=" << key << " error=" << head_resp.error_msg; if (head_resp.ret != ObjectStorageResponse::OK) { return head_resp; } diff --git a/cloud/src/recycler/s3_accessor.cpp b/cloud/src/recycler/s3_accessor.cpp index d902422e66ffbe..792cd11481c947 100644 --- a/cloud/src/recycler/s3_accessor.cpp +++ b/cloud/src/recycler/s3_accessor.cpp @@ -480,10 +480,10 @@ int S3Accessor::delete_files(const std::vector& paths) { } return obj_client_ - ->delete_objects(conf_.bucket, std::move(keys), - {.check_exists_before_delete = - config::enable_delete_file_check_object_exists, - .executor = worker_pool}) + ->delete_objects( + conf_.bucket, std::move(keys), + {.check_exists_before_delete = config::enable_delete_file_check_object_exists, + .executor = worker_pool}) .ret; } diff --git a/cloud/src/recycler/s3_obj_client.cpp b/cloud/src/recycler/s3_obj_client.cpp index d8d3a2a3af4ebb..bd4a9d5009b51c 100644 --- a/cloud/src/recycler/s3_obj_client.cpp +++ b/cloud/src/recycler/s3_obj_client.cpp @@ -264,8 +264,7 @@ ObjectStorageResponse S3ObjClient::delete_objects(const std::string& bucket, for (const auto& key : keys) { auto head_resp = head_object({.bucket = bucket, .key = key}, &obj_meta); DCHECK_EQ(head_resp.ret, ObjectStorageResponse::OK) - << "bucket=" << bucket << " key=" << key - << " error=" << head_resp.error_msg; + << "bucket=" << bucket << " key=" << key << " error=" << head_resp.error_msg; if (head_resp.ret != ObjectStorageResponse::OK) { return head_resp; } diff --git a/cloud/test/s3_accessor_client_test.cpp b/cloud/test/s3_accessor_client_test.cpp index 6f2c36e423bea3..66a58d86af9526 100644 --- a/cloud/test/s3_accessor_client_test.cpp +++ b/cloud/test/s3_accessor_client_test.cpp @@ -17,8 +17,8 @@ #include #include -#include #include +#include #include #include #include @@ -68,8 +68,8 @@ class MockDeleteS3Client : public Aws::S3::S3Client { return Aws::S3::Model::HeadObjectOutcome(Aws::S3::Model::HeadObjectResult()); } - auto error = Aws::Client::AWSError( - Aws::S3::S3Errors::RESOURCE_NOT_FOUND, false); + auto error = Aws::Client::AWSError(Aws::S3::S3Errors::RESOURCE_NOT_FOUND, + false); error.SetResponseCode(Aws::Http::HttpResponseCode::NOT_FOUND); return Aws::S3::Model::HeadObjectOutcome(error); }