Skip to content

Fix DDL catalog livelock fallout: conflict typing, Collection lifetime, cursor lifetime #491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
liunyl merged 10 commits into from
Aug 12, 2026
Merged

Fix DDL catalog livelock fallout: conflict typing, Collection lifetime, cursor lifetime #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f89e919
fix: surface catalog DDL conflicts as retryable WriteConflictException
liunyl Aug 11, 2026
004352c
fix: reference-count cached Collections and pin holders across catalo…
liunyl Aug 11, 2026
23daadf
fix: cursor lifetime across wire operations and concurrent DDL
liunyl Aug 11, 2026
ac58d0b
build: bump data_substrate to fix/catalog-write-intent-fail-fast
liunyl Aug 11, 2026
467593f
fix: abandon the snapshot before write-conflict backoff
liunyl Aug 11, 2026
2ea4f34
build: bump data_substrate for metadata-only tx close fix
liunyl Aug 12, 2026
6fc55dc
fix: close a stashed cursor's scan at detach, not at every save
liunyl Aug 12, 2026
50965d0
build: bump data_substrate for corrected catalog fail-fast rationale
liunyl Aug 12, 2026
a632b9a
build: repoint data_substrate at the post-merge follow-up branch
liunyl Aug 12, 2026
99266ec
build: bump data_substrate to tx_service main
liunyl Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/mongo/db/catalog/collection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,26 @@ class CappedInsertNotifier {
* this is NOT safe through a yield right now.
* not sure if it will be, or what yet.
*/
class Collection final : CappedCallback, UpdateNotifier {
class Collection final : CappedCallback,
UpdateNotifier,
public std::enable_shared_from_this<Collection> {
public:
using Uptr = std::unique_ptr<Collection>;
// Shared ownership is required for the Database collection caches: getCollection() rebuilds
// cached entries in place on catalog version changes while operations still hold raw pointers
// (pinned on their RecoveryUnit), so eviction must not destroy eagerly.
using Sptr = std::shared_ptr<Collection>;

/**
* Shared ownership of this object when it is owned by a shared collection cache
* (DatabaseImpl::_collections, UUIDCatalog); null for uniquely-owned instances. Used by
* holders that outlive the operation that resolved the collection -- e.g. globally-managed
* aggregation cursors, whose getMore path never re-resolves through getCollection() and so
* never takes a RecoveryUnit pin.
*/
Sptr sharedFromThisIfShared() {
return weak_from_this().lock();
}

enum ValidationAction { WARN, ERROR_V };
enum ValidationLevel { OFF, MODERATE, STRICT_V };
Expand Down
54 changes: 51 additions & 3 deletions src/mongo/db/catalog/collection_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,23 @@ Status CollectionImpl::insertDocumentsForOplog(OperationContext* opCtx,
return status;

opCtx->recoveryUnit()->onCommit(
[this](boost::optional<Timestamp>) { notifyCappedWaitersIfNeeded(); });
[notifier = _cappedNotifier](boost::optional<Timestamp>) {
// Capture the notifier, not the Collection. Eloq refreshes the cached Collection
// while committing catalog metadata, before RecoveryUnit callbacks run, so a
// captured `this` can already be freed here -- the same hazard the eloq guard in
// IndexCatalogEntryImpl::setMultikey was added for. Holding a shared_ptr copy keeps
// the notifier alive independently of the Collection.
//
// The haveCappedWaiters() short-circuit is dropped along with `this`: its use_count
// test cannot be expressed once the Collection may be gone, and it was only an
// optimisation. notifyAll() on a notifier nobody waits on takes one uncontended
// mutex and bumps a version that no waiter reads; on a killed notifier it is
// likewise harmless, because waiters exit on the _dead flag. Non-capped collections
// have a null notifier and do nothing at all.
if (notifier) {
notifier->notifyAll();
}
});
Comment on lines 337 to +354

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find remaining callers of the capped-waiter helpers.
rg -nP -C3 '\b(haveCappedWaiters|notifyCappedWaitersIfNeeded)\s*\(' --type=cpp --type=cpp-header

Repository: eloqdata/eloqdoc

Length of output: 193

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- helper definitions and callers ---'
rg -nP -C4 '\b(haveCappedWaiters|notifyCappedWaitersIfNeeded)\s*\(' --glob '*.cpp' --glob '*.h' --glob '*.hpp' --glob '*.inl' .
printf '%s\n' '--- definitions in collection implementation ---'
rg -n -C12 'haveCappedWaiters|notifyCappedWaitersIfNeeded' src/mongo/db/catalog/collection_impl.cpp src/mongo/db/catalog/collection_impl.h

Repository: eloqdata/eloqdoc

Length of output: 31521

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- oplog caller context ---'
sed -n '1665,1720p' src/mongo/db/repl/oplog.cpp
printf '%s\n' '--- WiredTiger oplog manager caller context ---'
sed -n '165,265p' src/mongo/db/storage/wiredtiger/wiredtiger_oplog_manager.cpp
printf '%s\n' '--- record-store capped callback plumbing ---'
rg -n -C5 'setCappedCallback|getCappedInsertNotifier|CappedCallback|_cappedNotifier' \
  src/mongo/db/catalog/collection_impl.cpp \
  src/mongo/db/catalog/collection_impl.h \
  src/mongo/db/storage \
  src/mongo/db/repl
printf '%s\n' '--- notifier implementation and use_count-related behavior ---'
rg -n -C8 'class CappedInsertNotifier|notifyAll\(|_dead|waitUntil' src/mongo/db

Repository: eloqdata/eloqdoc

Length of output: 50373

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- _localOplogCollection declarations and assignments ---'
rg -n -C4 '_localOplogCollection' src/mongo/db/repl/oplog.cpp src/mongo/db/repl/oplog.h
printf '%s\n' '--- production call sites only ---'
rg -nP '\b(haveCappedWaiters|notifyCappedWaitersIfNeeded)\s*\(' src/mongo \
  --glob '!**/*test*' --glob '!src/mongo/db/catalog/collection_impl.cpp' \
  --glob '!src/mongo/db/catalog/collection_impl.h' --glob '!src/mongo/db/catalog/collection.h'
printf '%s\n' '--- CollectionImpl notifier initialization and callback registration ---'
rg -n -C8 '_cappedNotifier|setCappedCallback|CappedCallback' \
  src/mongo/db/catalog/collection_impl.cpp src/mongo/db/catalog/collection_impl.h
printf '%s\n' '--- CollectionImpl insert method signatures and callback regions ---'
sed -n '285,430p' src/mongo/db/catalog/collection_impl.cpp

Repository: eloqdata/eloqdoc

Length of output: 29648

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- WiredTiger oplog manager decision and notification flow ---'
sed -n '175,258p' src/mongo/db/storage/wiredtiger/wiredtiger_oplog_manager.cpp
printf '%s\n' '--- WiredTiger record-store callback methods ---'
sed -n '1265,1312p' src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
printf '%s\n' '--- callback setup and wait lifecycle references ---'
rg -n -C6 'setCappedCallback|haveCappedWaiters\(\)|notifyCappedWaitersIfNeeded\(\)' \
  src/mongo/db/storage/wiredtiger src/mongo/db/catalog/collection_impl.cpp \
  --glob '!**/*test*'
printf '%s\n' '--- commit callback registration semantics ---'
rg -n -C6 'onCommit\(' src/mongo/db/recovery_unit.h src/mongo/db/storage/recovery_unit.h \
  src/mongo/db/modules/eloq/src

Repository: eloqdata/eloqdoc

Length of output: 23084

🏁 Script executed:

#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path

collection = Path("src/mongo/db/catalog/collection_impl.cpp").read_text()
record_store = Path("src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp").read_text()
oplog_manager = Path("src/mongo/db/storage/wiredtiger/wiredtiger_oplog_manager.cpp").read_text()
oplog = Path("src/mongo/db/repl/oplog.cpp").read_text()

assert collection.count("[notifier = _cappedNotifier]") == 3
assert "return _cappedNotifier.use_count() > 1;" in collection
assert "if (haveCappedWaiters())" in collection
assert "return _cappedCallback && _cappedCallback->haveCappedWaiters();" in record_store
assert "oplogRecordStore->haveCappedWaiters()" in oplog_manager
assert "oplogRecordStore->notifyCappedWaitersIfNeeded();" in oplog_manager
assert "_localOplogCollection->notifyCappedWaitersIfNeeded();" in oplog

print("pending commit notifier copies are counted by CollectionImpl::haveCappedWaiters")
print("WiredTigerOplogManager reaches CollectionImpl::haveCappedWaiters through WiredTigerRecordStore")
print("the waiter check controls the oplog journal-delay loop")
print("the notification path also invokes CollectionImpl::haveCappedWaiters")
PY

Repository: eloqdata/eloqdoc

Length of output: 453

Preserve the haveCappedWaiters() optimization
A pending commit callback makes CollectionImpl::haveCappedWaiters() return true without an await-data cursor. WiredTigerOplogManager uses this result to bypass its journal delay, which can cause unnecessary synchronization. Keep the waiter check in a lifetime-safe form, or track waiter presence separately from notifier ownership.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mongo/db/catalog/collection_impl.cpp` around lines 337 - 354, Preserve
the haveCappedWaiters() optimization in the onCommit callback without capturing
CollectionImpl or risking a dangling this pointer. Capture or record waiter
presence separately before registering the callback, then only call
notifier->notifyAll() when waiters were present and the notifier remains valid;
retain the existing null-notifier behavior.


return status;
}
Expand Down Expand Up @@ -388,7 +404,23 @@ Status CollectionImpl::insertDocuments(OperationContext* opCtx,
opCtx, ns(), uuid(), begin, end, fromMigrate);

opCtx->recoveryUnit()->onCommit(
[this](boost::optional<Timestamp>) { notifyCappedWaitersIfNeeded(); });
[notifier = _cappedNotifier](boost::optional<Timestamp>) {
// Capture the notifier, not the Collection. Eloq refreshes the cached Collection
// while committing catalog metadata, before RecoveryUnit callbacks run, so a
// captured `this` can already be freed here -- the same hazard the eloq guard in
// IndexCatalogEntryImpl::setMultikey was added for. Holding a shared_ptr copy keeps
// the notifier alive independently of the Collection.
//
// The haveCappedWaiters() short-circuit is dropped along with `this`: its use_count
// test cannot be expressed once the Collection may be gone, and it was only an
// optimisation. notifyAll() on a notifier nobody waits on takes one uncontended
// mutex and bumps a version that no waiter reads; on a killed notifier it is
// likewise harmless, because waiters exit on the _dead flag. Non-capped collections
// have a null notifier and do nothing at all.
if (notifier) {
notifier->notifyAll();
}
});

MONGO_FAIL_POINT_BLOCK(hangAfterCollectionInserts, extraData) {
const BSONObj& data = extraData.getData();
Expand Down Expand Up @@ -472,7 +504,23 @@ Status CollectionImpl::insertDocument(OperationContext* opCtx,
opCtx, ns(), uuid(), inserts.begin(), inserts.end(), false);

opCtx->recoveryUnit()->onCommit(
[this](boost::optional<Timestamp>) { notifyCappedWaitersIfNeeded(); });
[notifier = _cappedNotifier](boost::optional<Timestamp>) {
// Capture the notifier, not the Collection. Eloq refreshes the cached Collection
// while committing catalog metadata, before RecoveryUnit callbacks run, so a
// captured `this` can already be freed here -- the same hazard the eloq guard in
// IndexCatalogEntryImpl::setMultikey was added for. Holding a shared_ptr copy keeps
// the notifier alive independently of the Collection.
//
// The haveCappedWaiters() short-circuit is dropped along with `this`: its use_count
// test cannot be expressed once the Collection may be gone, and it was only an
// optimisation. notifyAll() on a notifier nobody waits on takes one uncontended
// mutex and bumps a version that no waiter reads; on a killed notifier it is
// likewise harmless, because waiters exit on the _dead flag. Non-capped collections
// have a null notifier and do nothing at all.
if (notifier) {
notifier->notifyAll();
}
});

return loc.getStatus();
}
Expand Down
11 changes: 7 additions & 4 deletions src/mongo/db/catalog/database.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ namespace mongo {
class Database : public Decorable<Database> {
public:
// Used for range-based loop only
using CollectionMapView = std::map<std::string, Collection::Uptr>;
using CollectionMap = std::map<std::string, Collection::Uptr, std::less<void>>;
using CollectionMapView = std::map<std::string, Collection::Sptr>;
using CollectionMap = std::map<std::string, Collection::Sptr, std::less<void>>;

class Impl {
public:
Expand Down Expand Up @@ -130,7 +130,10 @@ class Database : public Decorable<Database> {
virtual StatusWith<NamespaceString> makeUniqueCollectionNamespace(
OperationContext* opCtx, StringData collectionNameModel) = 0;

virtual CollectionMapView& collections(OperationContext* opCtx) = 0;
// Returned by value: an operation-owned snapshot. A shared member map would be cleared
// and rebuilt while earlier callers iterate it (map-iterator invalidation that shared_ptr
// entries do not cure).
virtual CollectionMapView collections(OperationContext* opCtx) = 0;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// virtual CollectionMap& collections() = 0;
// virtual const CollectionMap& collections() const = 0;
// virtual CollectionMap::const_iterator begin() const = 0;
Expand Down Expand Up @@ -219,7 +222,7 @@ class Database : public Decorable<Database> {
inline Database(Database&&) = delete;
inline Database& operator=(Database&&) = delete;

CollectionMapView& collections(OperationContext* opCtx) {
CollectionMapView collections(OperationContext* opCtx) {
return this->_impl().collections(opCtx);
}
// inline iterator begin() const {
Expand Down
167 changes: 123 additions & 44 deletions src/mongo/db/catalog/database_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,10 @@ void uassertNamespaceNotIndex(StringData ns, StringData caller) {
// };

DatabaseImpl::~DatabaseImpl() {
// Entries may be kept alive past this point by RecoveryUnit pins and cursor pins; clearing
// under the mutex only synchronises the map structure itself against background accessors.
stdx::lock_guard<stdx::mutex> lk(_collectionsMutex);
_collections.clear();
// for (CollectionMap::const_iterator i = _collections.begin(); i != _collections.end(); ++i)
// delete i->second;
}

void DatabaseImpl::close(OperationContext* opCtx, const std::string& reason) {
Expand All @@ -169,8 +170,17 @@ void DatabaseImpl::close(OperationContext* opCtx, const std::string& reason) {
// Clear cache of oplog Collection pointer.
repl::oplogCheckCloseDatabase(opCtx, this->_this);

for (const auto& [name, coll] : _collections) {
// auto coll = pair.second;
// Snapshot under the mutex, invalidate outside it; skip null entries (the map can hold them).
std::vector<Collection::Sptr> colls;
{
stdx::lock_guard<stdx::mutex> lk(_collectionsMutex);
for (const auto& [name, coll] : _collections) {
if (coll) {
colls.push_back(coll);
}
}
}
for (const auto& coll : colls) {
coll->getCursorManager()->invalidateAll(opCtx, true, reason);
}
}
Expand Down Expand Up @@ -249,32 +259,53 @@ Collection* DatabaseImpl::_getOrCreateCollectionInstance(OperationContext* opCtx
return coll;
}

Collection* DatabaseImpl::_createCollectionHandler(OperationContext* opCtx,
const NamespaceString& nss,
bool createIdIndex,
const BSONObj& idIndexSpec,
bool forView) {
MONGO_LOG(1) << "DatabaseImpl::_createCollectionHandler";
if (!forView) {
if (auto iter = _collections.find(nss.toString()); iter != _collections.end()) {
return iter->second.get();
}
}
Collection::Sptr DatabaseImpl::_buildCollectionInstance(OperationContext* opCtx,
const NamespaceString& nss,
CollectionOptions* optionsOut) {
auto cce = _dbEntry->getCollectionCatalogEntry(opCtx, nss.toStringData());
if (!cce) {
// The collection not exists in the Eloq
return nullptr;
}
CollectionCatalogEntry::MetaData metadata = cce->getMetaData(opCtx);
auto uuid = metadata.options.uuid;
CollectionCatalogEntry::MetaData metadata = cce->getMetaData(opCtx); // yields
auto rs = cce->getRecordStore();
auto collection =
std::make_unique<Collection>(opCtx, nss.toStringData(), uuid, cce, rs, _dbEntry);
auto collection = std::make_shared<Collection>(
opCtx, nss.toStringData(), metadata.options.uuid, cce, rs, _dbEntry);
if (optionsOut) {
*optionsOut = std::move(metadata.options);
}
return collection;
}

if (forView) {
_collectionsView.try_emplace(nss.toString(), std::move(collection));
Collection* DatabaseImpl::_createCollectionHandler(OperationContext* opCtx,
const NamespaceString& nss,
bool createIdIndex,
const BSONObj& idIndexSpec) {
MONGO_LOG(1) << "DatabaseImpl::_createCollectionHandler";
{
bool hasEntry = false;
Collection::Sptr existing;
{
stdx::lock_guard<stdx::mutex> lk(_collectionsMutex);
if (auto iter = _collections.find(nss.toString()); iter != _collections.end()) {
hasEntry = true;
existing = iter->second;
}
}
if (hasEntry) {
// The map can hold null entries; return whatever the entry holds.
if (existing && opCtx->recoveryUnit()) {
opCtx->recoveryUnit()->pinResource(existing);
}
return existing.get();
}
}
CollectionOptions options;
Collection::Sptr collection = _buildCollectionInstance(opCtx, nss, &options);
if (!collection) {
return nullptr;
}
auto uuid = options.uuid;

if (uuid) {
// We are not in a WUOW only when we are called from Database::init(). There is no need
Expand All @@ -290,8 +321,8 @@ Collection* DatabaseImpl::_createCollectionHandler(OperationContext* opCtx,
BSONObj fullIdIndexSpec;
if (createIdIndex) {
if (collection->requiresIdIndex()) {
if (metadata.options.autoIndexId == CollectionOptions::YES ||
metadata.options.autoIndexId == CollectionOptions::DEFAULT) {
if (options.autoIndexId == CollectionOptions::YES ||
options.autoIndexId == CollectionOptions::DEFAULT) {
// createCollection() may be called before the in-memory fCV parameter is
// initialized, so use the unsafe fCV getter here.

Expand Down Expand Up @@ -338,12 +369,23 @@ Collection* DatabaseImpl::_createCollectionHandler(OperationContext* opCtx,
// createSystemIndexes(opCtx, collection.get());
}

auto [iter, _] = _collections.try_emplace(nss.toString(), std::move(collection));

// The catalog read in _buildCollectionInstance can yield the coroutine, so another accessor
// may have rebuilt this entry meanwhile; try_emplace keeps the winner and our redundant
// instance is discarded.
{
stdx::lock_guard<stdx::mutex> lk(_collectionsMutex);
auto [iter, inserted] = _collections.try_emplace(nss.toString(), collection);
if (!inserted) {
collection = iter->second;
}
}
if (collection && opCtx->recoveryUnit()) {
opCtx->recoveryUnit()->pinResource(collection);
}

MONGO_LOG(1) << "[opID]=" << opCtx->getOpID() << "DatabaseImpl::createCollection"
<< ". create done and handler to collection is available";
return iter->second.get();
return collection.get();
}

DatabaseImpl::DatabaseImpl(Database* const this_,
Expand Down Expand Up @@ -769,19 +811,38 @@ Status DatabaseImpl::_finishDropCollection(OperationContext* opCtx,
void DatabaseImpl::_clearCollectionCache(OperationContext* opCtx,
StringData fullns,
const std::string& reason,
bool collectionGoingAway) {
bool collectionGoingAway,
const Collection* onlyIfIs) {
invariant(_name == nsToDatabaseSubstring(fullns));
auto it = _collections.find(fullns);
Collection::Sptr evicted;
{
stdx::lock_guard<stdx::mutex> lk(_collectionsMutex);
auto it = _collections.find(fullns);
if (it == _collections.end()) {
return;
}
if (onlyIfIs && it->second.get() != onlyIfIs) {
// A concurrent accessor already refreshed this entry; keep its rebuild.
return;
}
evicted = std::move(it->second);
_collections.erase(it);
}

if (it == _collections.end()) {
// The map can hold null entries -- getCollection() checks `it->second` before using it.
if (!evicted) {
return;
}

// Takes ownership of the collection
// opCtx->recoveryUnit()->registerChange(new RemoveCollectionChange(this, it->second));
evicted->getCursorManager()->invalidateAll(opCtx, collectionGoingAway, reason);

it->second->getCursorManager()->invalidateAll(opCtx, collectionGoingAway, reason);
_collections.erase(it);
// Destruction is deferred, not immediate: every operation that obtained this Collection via
// getCollection() holds its own pin, and pinning here also covers raw pointers this operation
// received down a call chain (e.g. _finishDropCollection's argument). The object dies when
// the last pinning operation's RecoveryUnit is reset.
if (auto* ru = opCtx->recoveryUnit()) {
ru->pinResource(std::move(evicted));
}
}

Collection* DatabaseImpl::getCollection(OperationContext* opCtx, StringData ns, bool isForWrite) {
Expand All @@ -804,25 +865,41 @@ Collection* DatabaseImpl::getCollection(OperationContext* opCtx,
<< " exists: " << exists << ", isForWrite: " << isForWrite;
uassertStatusOK(status);
if (!exists) {
// The table is gone from the transactional catalog (dropped in this transaction or on
// another node). Evict any stale cached entry so a later re-create of the same namespace
// cannot be served the old object; destruction is deferred via pinResource.
_clearCollectionCache(opCtx, nss.ns(), "collection no longer exists", true);
return nullptr;
}

if (auto it = _collections.find(nss.ns()); it != _collections.end() && it->second) {
auto found = it->second.get();

Collection::Sptr found;
{
stdx::lock_guard<stdx::mutex> lk(_collectionsMutex);
if (auto it = _collections.find(nss.ns()); it != _collections.end() && it->second) {
found = it->second;
}
}
if (found) {
if (found->catalogVersion() == version) {
NamespaceUUIDCache& cache = NamespaceUUIDCache::get(opCtx);
if (auto uuid = found->uuid()) {
cache.ensureNamespaceInCache(nss, uuid.get());
}
return found;
// A null RecoveryUnit only occurs on early-boot OperationContexts that never reach
// collection code; a future RU-less path reaching here would return an unpinned raw
// pointer, losing the lifetime protection.
if (auto* ru = opCtx->recoveryUnit()) {
ru->pinResource(found);
}
return found.get();
} else {
MONGO_LOG(1) << "nss: " << nss.toStringData()
<< " version changed. old: " << found->catalogVersion()
<< ", new: " << version;
auto& uuidCatalog = UUIDCatalog::get(opCtx);
uuidCatalog.removeUUIDCatalogEntry(found->uuid().get());
_clearCollectionCache(opCtx, nss.ns(), "collection version changed", true);
_clearCollectionCache(
opCtx, nss.ns(), "collection version changed", true, found.get());
Comment on lines 895 to +902

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Guard the optional UUID before removeUUIDCatalogEntry.

Line 902 calls found->uuid().get() without checking that the optional is engaged. Line 887 in the same function checks it with if (auto uuid = found->uuid()), so a disengaged UUID is reachable on this code path too. A collection without a UUID makes this dereference a disengaged boost::optional.

The version-mismatch path is now taken constantly under concurrent DDL, which increases exposure.

🐛 Proposed fix 
             auto& uuidCatalog = UUIDCatalog::get(opCtx);
-            uuidCatalog.removeUUIDCatalogEntry(found->uuid().get());
+            if (auto foundUuid = found->uuid()) {
+                uuidCatalog.removeUUIDCatalogEntry(foundUuid.get());
+            }
             _clearCollectionCache(
                 opCtx, nss.ns(), "collection version changed", true, found.get());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} else {
MONGO_LOG(1) << "nss: " << nss.toStringData()
<< " version changed. old: " << found->catalogVersion()
<< ", new: " << version;
auto& uuidCatalog = UUIDCatalog::get(opCtx);
uuidCatalog.removeUUIDCatalogEntry(found->uuid().get());
_clearCollectionCache(opCtx, nss.ns(), "collection version changed", true);
_clearCollectionCache(
opCtx, nss.ns(), "collection version changed", true, found.get());
} else {
MONGO_LOG(1) << "nss: " << nss.toStringData()
<< " version changed. old: " << found->catalogVersion()
<< ", new: " << version;
auto& uuidCatalog = UUIDCatalog::get(opCtx);
if (auto foundUuid = found->uuid()) {
uuidCatalog.removeUUIDCatalogEntry(foundUuid.get());
}
_clearCollectionCache(
opCtx, nss.ns(), "collection version changed", true, found.get());
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mongo/db/catalog/database_impl.cpp` around lines 897 - 904, Guard the
UUID optional in the version-mismatch branch before calling
UUIDCatalog::removeUUIDCatalogEntry. Reuse the established conditional pattern
from the nearby check in the same function, removing the catalog entry only when
found->uuid() is engaged, while preserving the subsequent _clearCollectionCache
call.

return _createCollectionHandler(opCtx, nss, false);
}
} else {
Expand Down Expand Up @@ -1516,21 +1593,23 @@ StatusWith<NamespaceString> DatabaseImpl::makeUniqueCollectionNamespace(
<< " attempts due to namespace conflicts with existing collections.");
}

DatabaseImpl::CollectionMapView& DatabaseImpl::collections(OperationContext* opCtx) {
DatabaseImpl::CollectionMapView DatabaseImpl::collections(OperationContext* opCtx) {
MONGO_LOG(1) << "DatabaseImpl::collections";

std::vector<std::string> collectionInStorageEngine;
_dbEntry->getCollectionNamespaces(collectionInStorageEngine);

_collectionsView.clear();

// Operation-owned snapshot; no shared state, so no lock. A shared member map here would be
// cleared and rebuilt under earlier callers' iterators.
CollectionMapView view;
for (auto& collectionName : collectionInStorageEngine) {
NamespaceString nss{std::move(collectionName)};
MONGO_LOG(1) << "nss: " << nss;
_createCollectionHandler(opCtx, nss, false, BSONObj{}, true);
if (auto coll = _buildCollectionInstance(opCtx, nss, nullptr)) {
view.try_emplace(nss.toString(), std::move(coll));
}
}

return _collectionsView;
return view;
}


Expand Down
Loading
Loading