Collection recycle bin feature, v2 by ktrushin · Pull Request #1799 · percona/percona-server-mongodb

ktrushin · 2026-04-16T09:21:24Z

No description provided.

ktrushin · 2026-04-16T09:23:16Z

+    auto periodicRunner = serviceContext->getPeriodicRunner();
+    if (!periodicRunner) {
+        return;
+    }


If the runner hasn't started, that error is silently ignored without even printing a warning to the log. We must handle that error in more reasonable way.

ktrushin · 2026-04-16T09:24:56Z

+    if (nss.isSystem())
+        return true;
+    if (nss.isLocalDB())
+        return true;
+    if (nss.isOnInternalDb())
+        return true;
+    if (nss.isOplog())
+        return true;
+    return false;


We can express the same idea in a more concise way:

Suggested change

if (nss.isSystem())

return true;

if (nss.isLocalDB())

return true;

if (nss.isOnInternalDb())

return true;

if (nss.isOplog())

return true;

return false;

return nss.isSystem() || nss.isLocalDB() || nss.isOnInternalDb() || nss.isOplog();

ktrushin · 2026-04-16T09:41:11Z

+    return NamespaceString::createNamespaceString_forTest(originalNss.dbName(), recycleBinColl);
+}
+
+boost::optional<RecycleBinEntryInfo> parseRecycleBinNss(const NamespaceString& nss) {


The task of parsing a string that follows a particular pattern is usually easily solved with regular expressions. Even though the regex-based approach may be not as efficient as purpose-tailored parsing code, we probably won't see any measurable performance difference anyway.

ktrushin · 2026-04-16T11:09:35Z

+        auto status = recycleBinRestore(opCtx, recycleBinNss, restoreAs);
+        uassertStatusOK(status);
+
+        auto parsed = parseRecycleBinNss(recycleBinNss);


The snippet calls parseRecycleBinNss twice: first time inside recycleBinRestore and the second time explicitly. The second time is redundant. We can just adjust the return time of recycleBinRestore so that it returns relevant info for both success and failure cases

Acquire ReplicationStateTransitionLockGuard (MODE_IX) before calling canAcceptWritesForDatabase to prevent a race where replication state could change between the check and subsequent write operations. Also fix toStringForLogging call to use the free function form. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

C++ unit tests cover makeRecycleBinNss/parseRecycleBinNss namespace generation and parsing (basic, UUID suffix, dotted collection names, invalid inputs, round-trip). JS integration tests cover drop interception, restore, restoreAs, namespace conflicts, collision handling, cross-db listing, index preservation, and retention expiry purge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Adds a “collection recycle bin” feature that intercepts collection drops, hides recycled collections from normal listings, provides admin commands to list/restore them, and runs a background purger to delete expired entries.

Changes:

Intercept dropCollection to rename collections into a system.recycle_bin.* namespace when enabled.
Add recycle-bin core implementation, server parameters, commands (undeleteCollection, listRecycleBin), and background purger wiring in mongod.
Add unit/JS tests and a resmoke suite for the new feature.

Reviewed changes

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

Show a summary per file

File	Description
src/mongo/util/version_impl.tpl.cpp	Advertises the new Percona feature flag `"RecycleBin"` in build info.
src/mongo/db/shard_role/shard_catalog/drop_collection.cpp	Hooks drop flow to redirect to recycle bin when enabled.
src/mongo/db/shard_role/shard_catalog/BUILD.bazel	Pulls recycle-bin sources into `catalog_helpers` build target.
src/mongo/db/shard_role/ddl/list_collections.cpp	Filters recycle-bin collections out of normal `listCollections` output.
src/mongo/db/recycle_bin/recycle_bin_test.cpp	Adds unit tests for namespace formatting/parsing helpers.
src/mongo/db/recycle_bin/recycle_bin_commands.cpp	Adds admin commands to restore and list recycle-bin entries.
src/mongo/db/recycle_bin/recycle_bin.idl	Defines `recycleBinEnabled` and `recycleBinRetentionSeconds` server parameters.
src/mongo/db/recycle_bin/recycle_bin.h	Declares recycle-bin APIs and types.
src/mongo/db/recycle_bin/recycle_bin.cpp	Implements drop interception, restore/list, and periodic purger.
src/mongo/db/recycle_bin/BUILD.bazel	Adds Bazel targets for IDL gen, commands library, and unit test.
src/mongo/db/namespace_string.h	Adds `NamespaceString::isRecycleBinCollection()`.
src/mongo/db/mongod_main.cpp	Starts/stops the recycle-bin purger during server lifecycle.
src/mongo/db/commands/BUILD.bazel	Wires recycle-bin commands into the server command build.
jstests/recycle_bin/recycle_bin.js	End-to-end JS tests for drop interception, list/restore, and purging.
buildscripts/resmokeconfig/suites/recycle_bin.yml	Adds a resmoke suite to run recycle-bin JS tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T23:05:48Z

+    } else {
+        recycleBinColl =
+            fmt::format("system.recycle_bin.{}.{}", originalNss.coll(), dropTimeSecs);
+    }
+
+    return NamespaceString::createNamespaceString_forTest(originalNss.dbName(), recycleBinColl);
+}
+


This file constructs new namespaces via NamespaceString::createNamespaceString_forTest(...), which is documented as “MUST only be used for tests” and bypasses tenant-id assertions. Please switch to NamespaceStringUtil APIs (e.g., deserialize/serialize helpers) for production namespace construction to avoid serverless/multitenancy correctness issues.

Copilot · 2026-04-16T23:05:48Z

+        targetNss = *restoreAs;
+    } else {
+        targetNss = NamespaceString::createNamespaceString_forTest(
+            recycleBinNss.dbName(), parsed->originalCollection);
+    }
+


recycleBinRestore() uses NamespaceString::createNamespaceString_forTest(...) to build the target namespace when restoreAs is not provided. This helper explicitly bypasses tenant-id checks and is meant for tests only; please use NamespaceStringUtil construction instead so restore works correctly under multitenancy/serverless invariants.

Copilot · 2026-04-16T23:05:48Z

+    Status checkAuthForOperation(OperationContext* opCtx,
+                                 const DatabaseName&,
+                                 const BSONObj& cmdObj) const override {
+        auto nsStr = cmdObj.firstElement().String();
+        auto nss = NamespaceString::createNamespaceString_forTest(boost::none, nsStr);
+        return AuthorizationSession::get(opCtx->getClient())
+                       ->isAuthorizedForActionsOnNamespace(nss, ActionType::dropCollection)
+                   ? Status::OK()
+                   : Status(ErrorCodes::Unauthorized, "Unauthorized");
+    }
+
+    bool run(OperationContext* opCtx,
+             const DatabaseName&,
+             const BSONObj& cmdObj,
+             BSONObjBuilder& result) override {
+        auto nsStr = cmdObj.firstElement().String();
+        auto recycleBinNss =
+            NamespaceString::createNamespaceString_forTest(boost::none, nsStr);
+
+        boost::optional<NamespaceString> restoreAs;
+        if (cmdObj.hasField("restoreAs")) {
+            restoreAs = NamespaceString::createNamespaceString_forTest(
+                boost::none, cmdObj["restoreAs"].String());
+        }
+


CmdUndeleteCollection parses namespaces with NamespaceString::createNamespaceString_forTest(...) even though this is a production command. The helper is documented as test-only and skips tenant-id assertions; use NamespaceStringUtil parsing instead (and validate the command element types) so the command behaves correctly in multitenant/serverless configurations.

Copilot · 2026-04-16T23:05:49Z

+        auto nss = NamespaceString::createNamespaceString_forTest(boost::none, nsStr);
+        return AuthorizationSession::get(opCtx->getClient())
+                       ->isAuthorizedForActionsOnNamespace(nss, ActionType::dropCollection)
+                   ? Status::OK()
+                   : Status(ErrorCodes::Unauthorized, "Unauthorized");


Authorization for undeleteCollection currently checks ActionType::dropCollection on the recycle-bin namespace. Restoring is implemented as a rename to a (possibly user-specified) target namespace, so this check does not cover required privileges on the destination (e.g., insert/createIndex, and potentially drop if overwriting) and doesn’t match existing rename authorization patterns. Consider delegating to the existing renameCollection auth helper (or mirroring its logic) using the parsed source and computed target namespace.

Suggested change

auto nss = NamespaceString::createNamespaceString_forTest(boost::none, nsStr);

return AuthorizationSession::get(opCtx->getClient())

->isAuthorizedForActionsOnNamespace(nss, ActionType::dropCollection)

? Status::OK()

: Status(ErrorCodes::Unauthorized, "Unauthorized");

auto recycleBinNss =

NamespaceString::createNamespaceString_forTest(boost::none, nsStr);

boost::optional<NamespaceString> restoreAs;

if (cmdObj.hasField("restoreAs")) {

restoreAs = NamespaceString::createNamespaceString_forTest(

boost::none, cmdObj["restoreAs"].String());

}

NamespaceString targetNss = [&]() {

if (restoreAs) {

return *restoreAs;

}

auto parsed = parseRecycleBinNss(recycleBinNss);

uassert(ErrorCodes::InvalidNamespace,

"Invalid recycle bin namespace",

static_cast<bool>(parsed));

return NamespaceString::createNamespaceString_forTest(

recycleBinNss.dbName(), parsed->originalCollection);

}();

auto* authSession = AuthorizationSession::get(opCtx->getClient());

const bool isAuthorized =

authSession->isAuthorizedForActionsOnNamespace(recycleBinNss, ActionType::find) &&

authSession->isAuthorizedForActionsOnNamespace(targetNss, ActionType::insert) &&

authSession->isAuthorizedForActionsOnNamespace(targetNss, ActionType::createIndex);

return isAuthorized ? Status::OK()

: Status(ErrorCodes::Unauthorized, "Unauthorized");

Copilot · 2026-04-16T23:05:49Z

        "unique_collection_name.cpp",
        "//src/mongo/db:collection_compact.cpp",
        "//src/mongo/db/collection_crud:capped_utils.cpp",
+        "//src/mongo/db/recycle_bin:recycle_bin.cpp",
+        "//src/mongo/db/recycle_bin:recycle_bin_gen",
    ],


catalog_helpers is pulling in //src/mongo/db/recycle_bin:recycle_bin.cpp (and the IDL gen target) directly via srcs. This makes recycle-bin functionality an implicit part of the shard_catalog library and forces unrelated dependents to inherit a large dependency surface. It would be cleaner to define a dedicated mongo_cc_library for recycle_bin core (recycle_bin.cpp + generated IDL) in src/mongo/db/recycle_bin/BUILD.bazel and depend on it from both catalog_helpers and recycle_bin_commands via deps.

Copilot · 2026-04-16T23:05:49Z

+idl_generator(
+    name = "recycle_bin_gen",
+    src = "recycle_bin.idl",
+    deps = [
+        "//src/mongo/db:basic_types_gen",
+    ],
+)
+
+mongo_cc_unit_test(
+    name = "recycle_bin_test",
+    srcs = [
+        "recycle_bin_test.cpp",
+    ],
+    tags = ["mongo_unittest_first_group"],
+    deps = [
+        "//src/mongo/db/shard_role/shard_catalog:catalog_helpers",
+    ],
+)
+
+mongo_cc_library(
+    name = "recycle_bin_commands",
+    srcs = [
+        "recycle_bin_commands.cpp",
+    ],
+    deps = [
+        "//src/mongo/db:commands",
+        "//src/mongo/db/auth",
+        "//src/mongo/db/auth:authprivilege",
+        "//src/mongo/db/shard_role/shard_catalog:catalog_helpers",
+    ],
+)


src/mongo/db/recycle_bin/BUILD.bazel defines commands and tests but no standalone library target for the core implementation in recycle_bin.cpp. Right now the core .cpp is being compiled indirectly via shard_catalog:catalog_helpers, which is a brittle coupling and makes ownership/deps unclear. Add a mongo_cc_library(name = "recycle_bin", srcs = ["recycle_bin.cpp", ":recycle_bin_gen"], ...) and use it as a dependency where needed instead of relying on transitive inclusion.

Copilot · 2026-04-16T23:05:49Z

+    // Check if lastSegment is numeric (timestamp) or hex (uuid suffix)
+    bool lastIsNumeric = !lastSegment.empty();
+    for (auto c : lastSegment) {
+        if (!std::isdigit(c)) {
+            lastIsNumeric = false;
+            break;
+        }
+    }
+
+    if (lastIsNumeric) {
+        // Format: <original_coll>.<timestamp>
+        try {
+            timestamp = std::stoll(std::string(lastSegment));
+        } catch (...) {
+            return boost::none;
+        }
+        originalColl = std::string(remainder.substr(0, lastDot));
+    } else {
+        // Format: <original_coll>.<timestamp>.<uuid_prefix>
+        auto secondLastDot = remainder.rfind('.', lastDot - 1);
+        if (secondLastDot == std::string::npos) {


parseRecycleBinNss() decides whether the last segment is a timestamp by checking if it is all digits. The UUID suffix used by makeRecycleBinNss() is 8 hex chars and can be digits-only, which would be mis-parsed as a timestamp and corrupt originalCollection/dropTimeSecs. Consider parsing from the end by first checking for an 8-char hex UUID segment (and only then consuming the preceding numeric timestamp), rather than treating any numeric last segment as a timestamp.

- Simplify isExcludedFromRecycleBin to single return expression (ktrushin) - Rewrite parseRecycleBinNss with std::regex, fixing UUID-all-digits ambiguity where an 8-char hex prefix of only digits was mis-parsed as a timestamp (ktrushin, Copilot) - Change recycleBinRestore return type to StatusWith<NamespaceString>, eliminating redundant parseRecycleBinNss call in command handler (ktrushin) - Log warning when periodic runner is unavailable in startRecycleBinPurger (ktrushin) - Replace createNamespaceString_forTest/createDatabaseName_forTest with production APIs (NamespaceStringUtil, DatabaseNameUtil) (Copilot) - Improve undeleteCollection auth: check find on source, insert+createIndex on target namespace (Copilot) - Add unit test for all-digit UUID prefix parsing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Document architecture, server parameters, admin commands, namespace format, drop interception and restore flows, background purger, and collection lifecycle. Includes mermaid diagrams for each. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Collection recycle bin feature, v2

c62a761

ktrushin commented Apr 16, 2026

View reviewed changes

plebioda and others added 2 commits April 16, 2026 15:46

ktrushin requested a review from Copilot April 16, 2026 23:00

Copilot started reviewing on behalf of ktrushin April 16, 2026 23:01 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

plebioda and others added 2 commits April 17, 2026 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Collection recycle bin feature, v2#1799

Collection recycle bin feature, v2#1799
ktrushin wants to merge 5 commits into
masterfrom
recycle_bin_2

ktrushin commented Apr 16, 2026

Uh oh!

ktrushin Apr 16, 2026

Uh oh!

ktrushin Apr 16, 2026

Uh oh!

ktrushin Apr 16, 2026

Uh oh!

ktrushin Apr 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Copilot AI Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ktrushin commented Apr 16, 2026

Uh oh!

ktrushin Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

ktrushin Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

ktrushin Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

ktrushin Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants