SERVER-59197: Delete findAndModify image entries when their corresponding session is deleted.

2 files changed, 56 insertions, 9 deletions

diff --git a/jstests/replsets/sessions_collection_reaping.js b/jstests/replsets/sessions_collection_reaping.js
index ff8a2798164..09c651535d5 100644
--- a/jstests/replsets/sessions_collection_reaping.js
+++ b/jstests/replsets/sessions_collection_reaping.js
@@ -20,7 +20,12 @@
             {/* secondary */ rsConfig: {priority: 0}},
             {/* arbiter */ rsConfig: {arbiterOnly: true}}
         ],
-        nodeOptions: {setParameter: {TransactionRecordMinimumLifetimeMinutes: 0}}
+        nodeOptions: {
+            setParameter: {
+                TransactionRecordMinimumLifetimeMinutes: 0,
+                storeFindAndModifyImagesInSideCollection: true
+            }
+        }
     });
     let nodes = replTest.startSet();
 
@@ -28,6 +33,7 @@
     let primary = replTest.getPrimary();
     let sessionsCollOnPrimary = primary.getDB("config").system.sessions;
     let transactionsCollOnPrimary = primary.getDB("config").transactions;
+    let imageCollOnPrimary = primary.getDB("config").image_collection;
 
     replTest.awaitSecondaryNodes();
     let secondary = replTest.getSecondary();
@@ -38,12 +44,18 @@
     const reapErrorMsgRegex =
         new RegExp("Sessions collection is not set up.*waiting until next sessions reap interval");
 
-    // Set up a session with a retryable write.
+    // Set up a session with a retryable insert and findAndModify.
     let session = primary.startSession({retryWrites: 1});
     assert.commandWorked(session.getDatabase(dbName)[collName].save({x: 1}));
+    let result = session.getDatabase(dbName)[collName].findAndModify(
+        {query: {x: 1}, update: {$set: {y: 1}}});
+    // The findAndModify helper returns the document and not the whole response. Assert on the value
+    // of `x`. Though it's expected a command error would become an exception that fails the test.
+    assert.eq(1, result['x'], "Whole result: " + result);
     assert.commandWorked(primary.adminCommand({refreshLogicalSessionCacheNow: 1}));
     assert.eq(1, sessionsCollOnPrimary.count());
     assert.eq(1, transactionsCollOnPrimary.count());
+    assert.eq(1, imageCollOnPrimary.find().itcount());
 
     // Remove the session doc so the session gets reaped when reapLogicalSessionCacheNow is run.
     assert.commandWorked(sessionsCollOnPrimary.remove({}));
@@ -77,6 +89,7 @@
         assert.commandWorked(primary.adminCommand({reapLogicalSessionCacheNow: 1}));
 
         assert.eq(0, transactionsCollOnPrimary.count());
+        assert.eq(0, imageCollOnPrimary.find().itcount());
     }
 
     replTest.stopSet();
diff --git a/src/mongo/db/session_catalog.cpp b/src/mongo/db/session_catalog.cpp
index 21b52a2af6f..ab47e27b674 100644
--- a/src/mongo/db/session_catalog.cpp
+++ b/src/mongo/db/session_catalog.cpp
@@ -79,14 +79,20 @@ int removeSessionsTransactionRecords(OperationContext* opCtx,
     if (expiredSessionIds.empty())
         return 0;
 
-    // Remove the session ids from the on-disk catalog
-    write_ops::Delete deleteOp(NamespaceString::kSessionTransactionsTableNamespace);
-    deleteOp.setWriteCommandBase([] {
+    // Remove findAndModify images that map to deleted sessions. We first delete any images
+    // belonging to sessions about to be reaped, followed by the sessions. This way if there's a
+    // failure, we'll only be left with sessions that have a dangling reference to an image. Session
+    // reaping will rediscover the sessions to delete and try again.
+    //
+    // We opt for this rather than performing the two sets of deletes in a single transaction simply
+    // to reduce code complexity.
+    write_ops::Delete imageDeleteOp(NamespaceString::kConfigImagesNamespace);
+    imageDeleteOp.setWriteCommandBase([] {
         write_ops::WriteCommandBase base;
         base.setOrdered(false);
         return base;
     }());
-    deleteOp.setDeletes([&] {
+    imageDeleteOp.setDeletes([&] {
         std::vector<write_ops::DeleteOpEntry> entries;
         for (const auto& lsid : expiredSessionIds) {
             entries.emplace_back([&] {
@@ -102,9 +108,8 @@ int removeSessionsTransactionRecords(OperationContext* opCtx,
     BSONObj result;
 
     DBDirectClient client(opCtx);
-    client.runCommand(NamespaceString::kSessionTransactionsTableNamespace.db().toString(),
-                      deleteOp.toBSON({}),
-                      result);
+    client.runCommand(
+        NamespaceString::kConfigImagesNamespace.db().toString(), imageDeleteOp.toBSON({}), result);
 
     BatchedCommandResponse response;
     std::string errmsg;
@@ -113,6 +118,35 @@ int removeSessionsTransactionRecords(OperationContext* opCtx,
             response.parseBSON(result, &errmsg));
     uassertStatusOK(response.getTopLevelStatus());
 
+    // Remove the session ids from the on-disk catalog
+    write_ops::Delete sessionDeleteOp(NamespaceString::kSessionTransactionsTableNamespace);
+    sessionDeleteOp.setWriteCommandBase([] {
+        write_ops::WriteCommandBase base;
+        base.setOrdered(false);
+        return base;
+    }());
+    sessionDeleteOp.setDeletes([&] {
+        std::vector<write_ops::DeleteOpEntry> entries;
+        for (const auto& lsid : expiredSessionIds) {
+            entries.emplace_back([&] {
+                write_ops::DeleteOpEntry entry;
+                entry.setQ(BSON(LogicalSessionRecord::kIdFieldName << lsid.toBSON()));
+                entry.setMulti(false);
+                return entry;
+            }());
+        }
+        return entries;
+    }());
+
+    client.runCommand(NamespaceString::kSessionTransactionsTableNamespace.db().toString(),
+                      sessionDeleteOp.toBSON({}),
+                      result);
+
+    uassert(ErrorCodes::FailedToParse,
+            str::stream() << "Failed to parse response " << result,
+            response.parseBSON(result, &errmsg));
+    uassertStatusOK(response.getTopLevelStatus());
+
     return response.getN();
 }