SERVER-32957 part 1: mongos will now destroy kill pending cursors on checkin

9 files changed, 130 insertions, 127 deletions

diff --git a/src/mongo/s/query/cluster_client_cursor.h b/src/mongo/s/query/cluster_client_cursor.h
index c3bd8579bbb..a3b680e9de3 100644
--- a/src/mongo/s/query/cluster_client_cursor.h
+++ b/src/mongo/s/query/cluster_client_cursor.h
@@ -90,6 +90,11 @@ public:
     virtual void detachFromOperationContext() = 0;
 
     /**
+     * Return the current context the cursor is attached to, if any.
+     */
+    virtual OperationContext* getCurrentOperationContext() const = 0;
+
+    /**
      * Returns whether or not this cursor is tailable.
      */
     virtual bool isTailable() const = 0;
diff --git a/src/mongo/s/query/cluster_client_cursor_impl.cpp b/src/mongo/s/query/cluster_client_cursor_impl.cpp
index daaf85a6f6f..e555f1d32e8 100644
--- a/src/mongo/s/query/cluster_client_cursor_impl.cpp
+++ b/src/mongo/s/query/cluster_client_cursor_impl.cpp
@@ -130,6 +130,10 @@ void ClusterClientCursorImpl::detachFromOperationContext() {
     _root->detachFromOperationContext();
 }
 
+OperationContext* ClusterClientCursorImpl::getCurrentOperationContext() const {
+    return _opCtx;
+}
+
 bool ClusterClientCursorImpl::isTailable() const {
     return _params.tailableMode != TailableMode::kNormal;
 }
diff --git a/src/mongo/s/query/cluster_client_cursor_impl.h b/src/mongo/s/query/cluster_client_cursor_impl.h
index 88b4b25306e..e2ffee6b869 100644
--- a/src/mongo/s/query/cluster_client_cursor_impl.h
+++ b/src/mongo/s/query/cluster_client_cursor_impl.h
@@ -97,6 +97,8 @@ public:
 
     void detachFromOperationContext() final;
 
+    OperationContext* getCurrentOperationContext() const final;
+
     bool isTailable() const final;
 
     bool isTailableAndAwaitData() const final;
diff --git a/src/mongo/s/query/cluster_client_cursor_mock.cpp b/src/mongo/s/query/cluster_client_cursor_mock.cpp
index c1ddc2d8c90..1fa270bfd7e 100644
--- a/src/mongo/s/query/cluster_client_cursor_mock.cpp
+++ b/src/mongo/s/query/cluster_client_cursor_mock.cpp
@@ -41,7 +41,7 @@ ClusterClientCursorMock::ClusterClientCursorMock(boost::optional<LogicalSessionI
     : _killCallback(std::move(killCallback)), _lsid(lsid) {}
 
 ClusterClientCursorMock::~ClusterClientCursorMock() {
-    invariant(_exhausted || _killed);
+    invariant((_exhausted && _remotesExhausted) || _killed);
 }
 
 StatusWith<ClusterQueryResult> ClusterClientCursorMock::next(
diff --git a/src/mongo/s/query/cluster_client_cursor_mock.h b/src/mongo/s/query/cluster_client_cursor_mock.h
index 55dee0d3e41..81fb52f15b4 100644
--- a/src/mongo/s/query/cluster_client_cursor_mock.h
+++ b/src/mongo/s/query/cluster_client_cursor_mock.h
@@ -51,9 +51,17 @@ public:
 
     void kill(OperationContext* opCtx) final;
 
-    void reattachToOperationContext(OperationContext* opCtx) final {}
+    void reattachToOperationContext(OperationContext* opCtx) final {
+        _opCtx = opCtx;
+    }
 
-    void detachFromOperationContext() final {}
+    void detachFromOperationContext() final {
+        _opCtx = nullptr;
+    }
+
+    OperationContext* getCurrentOperationContext() const final {
+        return _opCtx;
+    }
 
     bool isTailable() const final;
 
@@ -94,6 +102,8 @@ private:
     bool _remotesExhausted = true;
 
     boost::optional<LogicalSessionId> _lsid;
+
+    OperationContext* _opCtx = nullptr;
 };
 
 }  // namespace mongo
diff --git a/src/mongo/s/query/cluster_cursor_manager.cpp b/src/mongo/s/query/cluster_cursor_manager.cpp
index 20a84582c7f..6715964daa5 100644
--- a/src/mongo/s/query/cluster_cursor_manager.cpp
+++ b/src/mongo/s/query/cluster_cursor_manager.cpp
@@ -118,16 +118,6 @@ StatusWith<ClusterQueryResult> ClusterCursorManager::PinnedCursor::next(
     return _cursor->next(execContext);
 }
 
-void ClusterCursorManager::PinnedCursor::reattachToOperationContext(OperationContext* opCtx) {
-    invariant(_cursor);
-    _cursor->reattachToOperationContext(opCtx);
-}
-
-void ClusterCursorManager::PinnedCursor::detachFromOperationContext() {
-    invariant(_cursor);
-    _cursor->detachFromOperationContext();
-}
-
 bool ClusterCursorManager::PinnedCursor::isTailable() const {
     invariant(_cursor);
     return _cursor->isTailable();
@@ -179,15 +169,8 @@ Status ClusterCursorManager::PinnedCursor::setAwaitDataTimeout(Milliseconds awai
 void ClusterCursorManager::PinnedCursor::returnAndKillCursor() {
     invariant(_cursor);
 
-    // Inform the manager that the cursor should be killed.
-    invariantOK(_manager->killCursor(_nss, _cursorId));
-
-    // Return the cursor to the manager.  It will be deleted on the next call to
-    // ClusterCursorManager::reapZombieCursors().
-    //
-    // The value of the argument to returnCursor() doesn't matter; the cursor will be kept as a
-    // zombie.
-    returnCursor(CursorState::NotExhausted);
+    // Return the cursor as exhausted so that it's deleted immediately.
+    returnCursor(CursorState::Exhausted);
 }
 
 ClusterCursorManager::ClusterCursorManager(ClockSource* clockSource)
@@ -322,6 +305,7 @@ StatusWith<ClusterCursorManager::PinnedCursor> ClusterCursorManager::checkOutCur
         LogicalSessionCache::get(opCtx)->vivify(opCtx, cursor->getLsid().get());
     }
 
+    cursor->reattachToOperationContext(opCtx);
     return PinnedCursor(this, std::move(cursor), nss, cursorId);
 }
 
@@ -329,40 +313,36 @@ void ClusterCursorManager::checkInCursor(std::unique_ptr<ClusterClientCursor> cu
                                          const NamespaceString& nss,
                                          CursorId cursorId,
                                          CursorState cursorState) {
+    invariant(cursor);
     // Read the clock out of the lock.
     const auto now = _clockSource->now();
 
-    stdx::unique_lock<stdx::mutex> lk(_mutex);
+    // Detach the cursor from the operation which had checked it out.
+    OperationContext* opCtx = cursor->getCurrentOperationContext();
+    invariant(opCtx);
+    cursor->detachFromOperationContext();
 
-    invariant(cursor);
-
-    const bool remotesExhausted = cursor->remotesExhausted();
+    stdx::unique_lock<stdx::mutex> lk(_mutex);
 
     CursorEntry* entry = _getEntry(lk, nss, cursorId);
     invariant(entry);
 
+    // killPending will be true if killCursor() was called while the cursor was in use or if the
+    // ClusterCursorCleanupJob decided that it expired.
+    // TODO SERVER-32957: Remove the killPending flag.
+    const bool killPending = entry->getKillPending();
+
     entry->setLastActive(now);
     entry->returnCursor(std::move(cursor));
 
-    if (cursorState == CursorState::NotExhausted || entry->getKillPending()) {
+    if (cursorState == CursorState::NotExhausted && !killPending) {
+        // The caller may need the cursor again.
         return;
     }
 
-    if (!remotesExhausted) {
-        // The cursor still has open remote cursors that need to be cleaned up. Schedule for
-        // deletion by the reaper thread by setting the kill pending flag.
-        entry->setKillPending();
-        return;
-    }
-
-    // The cursor is exhausted, is not already scheduled for deletion, and does not have any
-    // remote cursor state left to clean up. We can delete the cursor right away.
-    auto detachedCursor = _detachCursor(lk, nss, cursorId);
-    invariantOK(detachedCursor.getStatus());
-
-    // Deletion of the cursor can happen out of the lock.
-    lk.unlock();
-    detachedCursor.getValue().reset();
+    // After detaching the cursor, the entry will be destroyed.
+    entry = nullptr;
+    detachAndKillCursor(std::move(lk), opCtx, nss, cursorId);
 }
 
 Status ClusterCursorManager::checkAuthForKillCursors(OperationContext* opCtx,
@@ -381,26 +361,53 @@ Status ClusterCursorManager::checkAuthForKillCursors(OperationContext* opCtx,
     return authChecker(entry->getAuthenticatedUsers());
 }
 
-Status ClusterCursorManager::killCursor(const NamespaceString& nss, CursorId cursorId) {
-    stdx::lock_guard<stdx::mutex> lk(_mutex);
+Status ClusterCursorManager::killCursor(OperationContext* opCtx,
+                                        const NamespaceString& nss,
+                                        CursorId cursorId) {
+    invariant(opCtx);
+
+    stdx::unique_lock<stdx::mutex> lk(_mutex);
 
     CursorEntry* entry = _getEntry(lk, nss, cursorId);
     if (!entry) {
         return cursorNotFoundStatus(nss, cursorId);
     }
 
-    // Interrupt any operation currently using the cursor.
+    // Interrupt any operation currently using the cursor, unless if it's the current operation.
     OperationContext* opUsingCursor = entry->getOperationUsingCursor();
+    entry->setKillPending();
     if (opUsingCursor) {
+        // The caller shouldn't need to call killCursor on their own cursor.
+        invariant(opUsingCursor != opCtx, "Cannot call killCursor() on your own cursor");
+
         stdx::lock_guard<Client> lk(*opUsingCursor->getClient());
         opUsingCursor->getServiceContext()->killOperation(opUsingCursor, ErrorCodes::CursorKilled);
+        // Don't delete the cursor, as an operation is using it. It will be cleaned up when the
+        // operation is done.
+        return Status::OK();
     }
 
-    entry->setKillPending();
+    // No one is using the cursor, so we destroy it.
+    detachAndKillCursor(std::move(lk), opCtx, nss, cursorId);
+
+    // We no longer hold the lock here.
 
     return Status::OK();
 }
 
+void ClusterCursorManager::detachAndKillCursor(stdx::unique_lock<stdx::mutex> lk,
+                                               OperationContext* opCtx,
+                                               const NamespaceString& nss,
+                                               CursorId cursorId) {
+    auto detachedCursor = _detachCursor(lk, nss, cursorId);
+    invariantOK(detachedCursor.getStatus());
+
+    // Deletion of the cursor can happen out of the lock.
+    lk.unlock();
+    detachedCursor.getValue()->kill(opCtx);
+    detachedCursor.getValue().reset();
+}
+
 void ClusterCursorManager::killMortalCursorsInactiveSince(Date_t cutoff) {
     stdx::lock_guard<stdx::mutex> lk(_mutex);
 
@@ -561,7 +568,7 @@ std::vector<GenericCursor> ClusterCursorManager::getAllCursors() const {
 std::pair<Status, int> ClusterCursorManager::killCursorsWithMatchingSessions(
     OperationContext* opCtx, const SessionKiller::Matcher& matcher) {
     auto eraser = [&](ClusterCursorManager& mgr, CursorId id) {
-        uassertStatusOK(mgr.killCursor(getNamespaceForCursorId(id).get(), id));
+        uassertStatusOK(mgr.killCursor(opCtx, getNamespaceForCursorId(id).get(), id));
     };
 
     auto visitor = makeKillSessionsCursorManagerVisitor(opCtx, matcher, std::move(eraser));
diff --git a/src/mongo/s/query/cluster_cursor_manager.h b/src/mongo/s/query/cluster_cursor_manager.h
index 923e6914953..4f172cb9715 100644
--- a/src/mongo/s/query/cluster_cursor_manager.h
+++ b/src/mongo/s/query/cluster_cursor_manager.h
@@ -127,10 +127,11 @@ public:
      * A PinnedCursor can either be in a state where it owns a cursor, or can be in a null state
      * where it owns no cursor.  If a cursor is owned, the underlying cursor can be iterated with
      * next(), and the underlying cursor can be returned to the manager with the returnCursor()
-     * method (and after it is returned, no cursor will be owned).
+     * method (and after it is returned, no cursor will be owned). When a PinnedCursor is created,
+     * the underlying cursor is attached to the current OperationContext.
      *
-     * Invoking the PinnedCursor's destructor while it owns a cursor will kill and return the
-     * cursor.
+     * Invoking the PinnedCursor's destructor while it owns a cursor will kill, detach from the
+     * current OperationContext, and return the cursor.
      */
     class PinnedCursor {
         MONGO_DISALLOW_COPYING(PinnedCursor);
@@ -167,17 +168,6 @@ public:
         StatusWith<ClusterQueryResult> next(RouterExecStage::ExecContext);
 
         /**
-         * Sets the operation context for the cursor. Must be called before the first call to
-         * next().
-         */
-        void reattachToOperationContext(OperationContext* opCtx);
-
-        /**
-         * Detaches the cursor from its current OperationContext.
-         */
-        void detachFromOperationContext();
-
-        /**
          * Returns whether or not the underlying cursor is tailing a capped collection.  Cannot be
          * called after returnCursor() is called.  A cursor must be owned.
          */
@@ -191,8 +181,9 @@ public:
         bool isTailableAndAwaitData() const;
 
         /**
-         * Transfers ownership of the underlying cursor back to the manager.  A cursor must be
-         * owned, and a cursor will no longer be owned after this method completes.
+         * Transfers ownership of the underlying cursor back to the manager, and detaches it from
+         * the current OperationContext. A cursor must be owned, and a cursor will no longer be
+         * owned after this method completes.
          *
          * If 'Exhausted' is passed, the manager will de-register and destroy the cursor after it
          * is returned.
@@ -310,6 +301,8 @@ public:
      * returns an error Status with code CursorInUse.  If the given cursor is not registered or has
      * a pending kill, returns an error Status with code CursorNotFound.
      *
+     * Checking out a cursor will attach it to the given operation context.
+     *
      * 'authChecker' is function that will be called with the list of users authorized to use this
      * cursor. This function should check whether the current client is also authorized to use this
      * cursor, and if not, return an error status, which will cause checkOutCursor to fail.
@@ -342,9 +335,12 @@ public:
      * If the given cursor is not registered, returns an error Status with code CursorNotFound.
      * Otherwise, marks the cursor as 'kill pending' and returns Status::OK().
      *
+     * A thread which is currently using a cursor may not call killCursor() on it, but rather
+     * should kill the cursor by checking it back into the manager in the exhausted state.
+     *
      * Does not block.
      */
-    Status killCursor(const NamespaceString& nss, CursorId cursorId);
+    Status killCursor(OperationContext* opCtx, const NamespaceString& nss, CursorId cursorId);
 
     /**
      * Informs the manager that all mortal cursors with a 'last active' time equal to or earlier
@@ -445,6 +441,14 @@ private:
                        CursorState cursorState);
 
     /**
+     * Will detach a cursor, release the lock and then call kill() on it.
+     */
+    void detachAndKillCursor(stdx::unique_lock<stdx::mutex> lk,
+                             OperationContext* opCtx,
+                             const NamespaceString& nss,
+                             CursorId cursorId);
+
+    /**
      * Returns a pointer to the CursorEntry for the given cursor.  If the given cursor is not
      * registered, returns null.
      *
diff --git a/src/mongo/s/query/cluster_cursor_manager_test.cpp b/src/mongo/s/query/cluster_cursor_manager_test.cpp
index 0151800a799..d9cc6bcd8e3 100644
--- a/src/mongo/s/query/cluster_cursor_manager_test.cpp
+++ b/src/mongo/s/query/cluster_cursor_manager_test.cpp
@@ -117,7 +117,7 @@ protected:
 
         auto killCursorOpCtx = killCursorClient->makeOperationContext();
         invariant(killCursorOpCtx);
-        ASSERT_OK(getManager()->killCursor(nss, cursorId));
+        ASSERT_OK(getManager()->killCursor(killCursorOpCtx.get(), nss, cursorId));
 
         // Restore the old client. We don't need 'killCursorClient' anymore.
         killCursorOpCtx.reset();
@@ -276,7 +276,7 @@ TEST_F(ClusterCursorManagerTest, CheckOutCursorKilled) {
                                                ClusterCursorManager::CursorType::SingleTarget,
                                                ClusterCursorManager::CursorLifetime::Mortal,
                                                UserNameIterator()));
-    ASSERT_OK(getManager()->killCursor(nss, cursorId));
+    killCursorFromDifferentOpCtx(nss, cursorId);
     ASSERT_EQ(
         ErrorCodes::CursorNotFound,
         getManager()->checkOutCursor(nss, cursorId, _opCtx.get(), successAuthChecker).getStatus());
@@ -379,8 +379,22 @@ TEST_F(ClusterCursorManagerTest, ReturnCursorUpdateActiveTime) {
     getManager()->reapZombieCursors(nullptr);
     ASSERT(!isMockCursorKilled(0));
 }
+
 // Test that killing a pinned cursor by id successfully kills the cursor.
-TEST_F(ClusterCursorManagerTest, KillCursorBasic) {
+TEST_F(ClusterCursorManagerTest, KillUnpinnedCursorBasic) {
+    auto cursorId =
+        assertGet(getManager()->registerCursor(_opCtx.get(),
+                                               allocateMockCursor(),
+                                               nss,
+                                               ClusterCursorManager::CursorType::SingleTarget,
+                                               ClusterCursorManager::CursorLifetime::Mortal,
+                                               UserNameIterator()));
+    killCursorFromDifferentOpCtx(nss, cursorId);
+    ASSERT(isMockCursorKilled(0));
+}
+
+// Test that killing a pinned cursor by id successfully kills the cursor.
+TEST_F(ClusterCursorManagerTest, KillPinnedCursorBasic) {
     auto cursorId =
         assertGet(getManager()->registerCursor(_opCtx.get(),
                                                allocateMockCursor(),
@@ -396,9 +410,8 @@ TEST_F(ClusterCursorManagerTest, KillCursorBasic) {
     // When the cursor is pinned the operation which checked out the cursor should be interrupted.
     ASSERT_EQ(_opCtx->checkForInterruptNoAssert(), ErrorCodes::CursorKilled);
 
-    pinnedCursor.getValue().returnCursor(ClusterCursorManager::CursorState::NotExhausted);
     ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors(nullptr);
+    pinnedCursor.getValue().returnCursor(ClusterCursorManager::CursorState::NotExhausted);
     ASSERT(isMockCursorKilled(0));
 }
 
@@ -419,16 +432,14 @@ TEST_F(ClusterCursorManagerTest, KillCursorMultipleCursors) {
     }
     // Kill each cursor and verify that it was successfully killed.
     for (size_t i = 0; i < numCursors; ++i) {
-        ASSERT_OK(getManager()->killCursor(nss, cursorIds[i]));
-        ASSERT(!isMockCursorKilled(i));
-        getManager()->reapZombieCursors(nullptr);
+        ASSERT_OK(getManager()->killCursor(_opCtx.get(), nss, cursorIds[i]));
         ASSERT(isMockCursorKilled(i));
     }
 }
 
 // Test that killing an unknown cursor returns an error with code ErrorCodes::CursorNotFound.
 TEST_F(ClusterCursorManagerTest, KillCursorUnknown) {
-    Status killResult = getManager()->killCursor(nss, 5);
+    Status killResult = getManager()->killCursor(_opCtx.get(), nss, 5);
     ASSERT_EQ(ErrorCodes::CursorNotFound, killResult);
 }
 
@@ -444,7 +455,7 @@ TEST_F(ClusterCursorManagerTest, KillCursorWrongNamespace) {
                                                ClusterCursorManager::CursorType::SingleTarget,
                                                ClusterCursorManager::CursorLifetime::Mortal,
                                                UserNameIterator()));
-    Status killResult = getManager()->killCursor(incorrectNamespace, cursorId);
+    Status killResult = getManager()->killCursor(_opCtx.get(), incorrectNamespace, cursorId);
     ASSERT_EQ(ErrorCodes::CursorNotFound, killResult);
 }
 
@@ -458,7 +469,7 @@ TEST_F(ClusterCursorManagerTest, KillCursorWrongCursorId) {
                                                ClusterCursorManager::CursorType::SingleTarget,
                                                ClusterCursorManager::CursorLifetime::Mortal,
                                                UserNameIterator()));
-    Status killResult = getManager()->killCursor(nss, cursorId + 1);
+    Status killResult = getManager()->killCursor(_opCtx.get(), nss, cursorId + 1);
     ASSERT_EQ(ErrorCodes::CursorNotFound, killResult);
 }
 
@@ -582,22 +593,6 @@ TEST_F(ClusterCursorManagerTest, KillAllCursors) {
     }
 }
 
-// Test that reaping correctly calls kill() on the underlying ClusterClientCursor for a killed
-// cursor.
-TEST_F(ClusterCursorManagerTest, ReapZombieCursorsBasic) {
-    auto cursorId =
-        assertGet(getManager()->registerCursor(_opCtx.get(),
-                                               allocateMockCursor(),
-                                               nss,
-                                               ClusterCursorManager::CursorType::SingleTarget,
-                                               ClusterCursorManager::CursorLifetime::Mortal,
-                                               UserNameIterator()));
-    ASSERT_OK(getManager()->killCursor(nss, cursorId));
-    ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors(nullptr);
-    ASSERT(isMockCursorKilled(0));
-}
-
 // Test that reaping does not call kill() on the underlying ClusterClientCursor for a killed cursor
 // that is still pinned.
 TEST_F(ClusterCursorManagerTest, ReapZombieCursorsSkipPinned) {
@@ -709,7 +704,7 @@ TEST_F(ClusterCursorManagerTest, StatsKillShardedCursor) {
                                                ClusterCursorManager::CursorLifetime::Mortal,
                                                UserNameIterator()));
     ASSERT_EQ(1U, getManager()->stats().cursorsMultiTarget);
-    ASSERT_OK(getManager()->killCursor(nss, cursorId));
+    ASSERT_OK(getManager()->killCursor(_opCtx.get(), nss, cursorId));
     ASSERT_EQ(0U, getManager()->stats().cursorsMultiTarget);
 }
 
@@ -723,7 +718,7 @@ TEST_F(ClusterCursorManagerTest, StatsKillNotShardedCursor) {
                                                ClusterCursorManager::CursorLifetime::Mortal,
                                                UserNameIterator()));
     ASSERT_EQ(1U, getManager()->stats().cursorsSingleTarget);
-    ASSERT_OK(getManager()->killCursor(nss, cursorId));
+    ASSERT_OK(getManager()->killCursor(_opCtx.get(), nss, cursorId));
     ASSERT_EQ(0U, getManager()->stats().cursorsSingleTarget);
 }
 
@@ -935,19 +930,14 @@ TEST_F(ClusterCursorManagerTest, PinnedCursorReturnCursorExhausted) {
     registeredCursor.getValue().returnCursor(ClusterCursorManager::CursorState::Exhausted);
     ASSERT_EQ(0, registeredCursor.getValue().getCursorId());
 
-    // Cursor should have been destroyed without ever being killed. To be sure that the cursor has
-    // not been marked kill pending but not yet destroyed (i.e. that the cursor is not a zombie), we
-    // reapZombieCursors() and check that the cursor still has not been killed.
+    // Cursor should have been killed and destroyed.
     ASSERT_NOT_OK(
         getManager()->checkOutCursor(nss, cursorId, _opCtx.get(), successAuthChecker).getStatus());
-    ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors(nullptr);
-    ASSERT(!isMockCursorKilled(0));
+    ASSERT(isMockCursorKilled(0));
 }
 
 // Test that when a cursor is returned as exhausted but is still managing non-exhausted remote
-// cursors, the cursor is not destroyed immediately. Instead, it should be marked kill pending, and
-// should be killed and destroyed by reapZombieCursors().
+// cursors, the cursor is destroyed immediately.
 TEST_F(ClusterCursorManagerTest, PinnedCursorReturnCursorExhaustedWithNonExhaustedRemotes) {
     auto mockCursor = allocateMockCursor();
 
@@ -971,12 +961,10 @@ TEST_F(ClusterCursorManagerTest, PinnedCursorReturnCursorExhaustedWithNonExhaust
     registeredCursor.getValue().returnCursor(ClusterCursorManager::CursorState::Exhausted);
     ASSERT_EQ(0, registeredCursor.getValue().getCursorId());
 
-    // Cursor should be kill pending, so it will be killed during reaping.
+    // Cursor should be killed as soon as it's checked in.
+    ASSERT(isMockCursorKilled(0));
     ASSERT_NOT_OK(
         getManager()->checkOutCursor(nss, cursorId, _opCtx.get(), successAuthChecker).getStatus());
-    ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors(nullptr);
-    ASSERT(isMockCursorKilled(0));
 }
 
 // Test that the PinnedCursor move assignment operator correctly kills the cursor if it has not yet
@@ -992,8 +980,6 @@ TEST_F(ClusterCursorManagerTest, PinnedCursorMoveAssignmentKill) {
     auto pinnedCursor =
         getManager()->checkOutCursor(nss, cursorId, _opCtx.get(), successAuthChecker);
     pinnedCursor = ClusterCursorManager::PinnedCursor();
-    ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors(nullptr);
     ASSERT(isMockCursorKilled(0));
 }
 
@@ -1010,8 +996,6 @@ TEST_F(ClusterCursorManagerTest, PinnedCursorDestructorKill) {
         auto pinnedCursor =
             getManager()->checkOutCursor(nss, cursorId, _opCtx.get(), successAuthChecker);
     }
-    ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors(nullptr);
     ASSERT(isMockCursorKilled(0));
 }
 
@@ -1056,10 +1040,8 @@ TEST_F(ClusterCursorManagerTest, DoNotReapKilledPinnedCursors) {
     getManager()->reapZombieCursors(nullptr);
     ASSERT(!isMockCursorKilled(0));
 
-    // The cursor can be reaped once it is returned to the manager.
+    // The cursor can be destroyed once it is returned to the manager.
     pinnedCursor.getValue().returnCursor(ClusterCursorManager::CursorState::NotExhausted);
-    ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors(nullptr);
     ASSERT(isMockCursorKilled(0));
 }
 
@@ -1148,7 +1130,7 @@ TEST_F(ClusterCursorManagerTest, OneCursorWithASession) {
     ASSERT(cursors.find(cursorId) != cursors.end());
 
     // Remove the cursor from the manager.
-    ASSERT_OK(getManager()->killCursor(nss, cursorId));
+    ASSERT_OK(getManager()->killCursor(_opCtx.get(), nss, cursorId));
 
     // There should be no more cursor entries by session id.
     LogicalSessionIdSet sessions;
@@ -1213,7 +1195,7 @@ TEST_F(ClusterCursorManagerTest, MultipleCursorsWithSameSession) {
     ASSERT(cursors.find(cursorId2) != cursors.end());
 
     // Remove one cursor from the manager.
-    ASSERT_OK(getManager()->killCursor(nss, cursorId1));
+    ASSERT_OK(getManager()->killCursor(_opCtx.get(), nss, cursorId1));
 
     // Should still be able to retrieve the session.
     lsids.clear();
diff --git a/src/mongo/s/query/cluster_find.cpp b/src/mongo/s/query/cluster_find.cpp
index 1ce61880996..2a2521ef4fa 100644
--- a/src/mongo/s/query/cluster_find.cpp
+++ b/src/mongo/s/query/cluster_find.cpp
@@ -434,15 +434,6 @@ StatusWith<CursorResponse> ClusterFind::runGetMore(OperationContext* opCtx,
     long long startingFrom = pinnedCursor.getValue().getNumReturnedSoFar();
     auto cursorState = ClusterCursorManager::CursorState::NotExhausted;
 
-    pinnedCursor.getValue().reattachToOperationContext(opCtx);
-
-    // A pinned cursor will not be destroyed immediately if an exception is thrown. Instead it will
-    // be marked as killed, then reaped by a background thread later. If this happens, we want to be
-    // sure the cursor does not have a pointer to this OperationContext, since it will be destroyed
-    // as soon as we return, but the cursor will live on a bit longer.
-    ScopeGuard cursorDetach =
-        MakeGuard([&pinnedCursor]() { pinnedCursor.getValue().detachFromOperationContext(); });
-
     while (!FindCommon::enoughForGetMore(batchSize, batch.size())) {
         auto context = batch.empty()
             ? RouterExecStage::ExecContext::kGetMoreNoResultsYet
@@ -489,10 +480,8 @@ StatusWith<CursorResponse> ClusterFind::runGetMore(OperationContext* opCtx,
         batch.push_back(std::move(*next.getValue().getResult()));
     }
 
-    // Upon successful completion, we need to detach from the operation and transfer ownership of
-    // the cursor back to the cursor manager.
-    cursorDetach.Dismiss();
-    pinnedCursor.getValue().detachFromOperationContext();
+    // Upon successful completion, transfer ownership of the cursor back to the cursor manager. If
+    // the cursor has been exhausted, the cursor manager will clean it up for us.
     pinnedCursor.getValue().returnCursor(cursorState);
 
     CursorId idToReturn = (cursorState == ClusterCursorManager::CursorState::Exhausted)