From 62475e178de5278da0a75ac3f31f5ebc924e9896 Mon Sep 17 00:00:00 2001 From: Benjamin Frueh Date: Fri, 19 Sep 2025 11:47:09 +0200 Subject: [PATCH 1/6] Fix: Add cleanup for orphaned text steps Signed-off-by: Benjamin Frueh --- lib/Cron/Cleanup.php | 4 ++++ lib/Db/SessionMapper.php | 40 ++++++++++++++++++++++++++++++++++ lib/Service/SessionService.php | 4 ++++ 3 files changed, 48 insertions(+) diff --git a/lib/Cron/Cleanup.php b/lib/Cron/Cleanup.php index 0bbadbc28a2..9a8c6b61a0d 100644 --- a/lib/Cron/Cleanup.php +++ b/lib/Cron/Cleanup.php @@ -54,6 +54,10 @@ protected function run($argument): void { $removedSessions = $this->sessionService->removeInactiveSessionsWithoutSteps(); $this->logger->debug('Removed ' . $removedSessions . ' inactive sessions'); + $this->logger->debug('Run cleanup job for orphaned steps'); + $removedSteps = $this->sessionService->removeOrphanedSteps(); + $this->logger->debug('Removed ' . $removedSteps . ' orphaned steps'); + $this->logger->debug('Run cleanup job for obsolete documents folders'); $this->documentService->cleanupOldDocumentsFolders(); } diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php index 9c546e22a5b..45c257c553c 100644 --- a/lib/Db/SessionMapper.php +++ b/lib/Db/SessionMapper.php @@ -142,6 +142,46 @@ public function deleteInactiveWithoutSteps(?int $documentId = null): int { return $deletedCount; } + public function deleteOrphanedSteps(): int { + $startTime = microtime(true); + $maxExecutionSeconds = 30; + $batchSize = 1000; + $deletedCount = 0; + + do { + try { + $orphanedStepsBuilder = $this->db->getQueryBuilder(); + $orphanedStepsBuilder->select('st.id') + ->from('text_steps', 'st') + ->leftJoin('st', 'text_sessions', 's', $orphanedStepsBuilder->expr()->eq('st.session_id', 's.id')) + ->where($orphanedStepsBuilder->expr()->isNull('s.id')) + ->setMaxResults($batchSize); + + $result = $orphanedStepsBuilder->executeQuery(); + $stepIds = array_map(function ($row) { + return (int)$row['id']; + }, $result->fetchAll()); + $result->closeCursor(); + + if (empty($stepIds)) { + break; + } + + $deleteBuilder = $this->db->getQueryBuilder(); + $batchDeleted = $deleteBuilder->delete('text_steps') + ->where($deleteBuilder->expr()->in('id', $deleteBuilder->createParameter('ids'), IQueryBuilder::PARAM_INT_ARRAY)) + ->setParameter('ids', $stepIds, IQueryBuilder::PARAM_INT_ARRAY) + ->executeStatement(); + + $deletedCount += $batchDeleted; + } catch (\Exception) { + break; + } + } while ((microtime(true) - $startTime) < $maxExecutionSeconds); + + return $deletedCount; + } + public function deleteByDocumentId(int $documentId): int { $qb = $this->db->getQueryBuilder(); $qb->delete($this->getTableName()) diff --git a/lib/Service/SessionService.php b/lib/Service/SessionService.php index 55eed4332a5..5b7dcfabd37 100644 --- a/lib/Service/SessionService.php +++ b/lib/Service/SessionService.php @@ -138,6 +138,10 @@ public function removeInactiveSessionsWithoutSteps(?int $documentId = null): int return $this->sessionMapper->deleteInactiveWithoutSteps($documentId); } + public function removeOrphanedSteps(): int { + return $this->sessionMapper->deleteOrphanedSteps(); + } + public function getSession(int $documentId, int $sessionId, string $token): ?Session { if ($this->session !== null) { return $this->session; From c8b05f3e7a2005cf2bc51a70427558e8cc500001 Mon Sep 17 00:00:00 2001 From: Benjamin Frueh Date: Sat, 20 Sep 2025 22:51:31 +0200 Subject: [PATCH 2/6] Remove try-catch from deleteOrphanedSteps for consistency Signed-off-by: Benjamin Frueh --- lib/Db/SessionMapper.php | 48 ++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php index 45c257c553c..c4c67bb963f 100644 --- a/lib/Db/SessionMapper.php +++ b/lib/Db/SessionMapper.php @@ -149,34 +149,30 @@ public function deleteOrphanedSteps(): int { $deletedCount = 0; do { - try { - $orphanedStepsBuilder = $this->db->getQueryBuilder(); - $orphanedStepsBuilder->select('st.id') - ->from('text_steps', 'st') - ->leftJoin('st', 'text_sessions', 's', $orphanedStepsBuilder->expr()->eq('st.session_id', 's.id')) - ->where($orphanedStepsBuilder->expr()->isNull('s.id')) - ->setMaxResults($batchSize); - - $result = $orphanedStepsBuilder->executeQuery(); - $stepIds = array_map(function ($row) { - return (int)$row['id']; - }, $result->fetchAll()); - $result->closeCursor(); - - if (empty($stepIds)) { - break; - } - - $deleteBuilder = $this->db->getQueryBuilder(); - $batchDeleted = $deleteBuilder->delete('text_steps') - ->where($deleteBuilder->expr()->in('id', $deleteBuilder->createParameter('ids'), IQueryBuilder::PARAM_INT_ARRAY)) - ->setParameter('ids', $stepIds, IQueryBuilder::PARAM_INT_ARRAY) - ->executeStatement(); - - $deletedCount += $batchDeleted; - } catch (\Exception) { + $orphanedStepsBuilder = $this->db->getQueryBuilder(); + $orphanedStepsBuilder->select('st.id') + ->from('text_steps', 'st') + ->leftJoin('st', 'text_sessions', 's', $orphanedStepsBuilder->expr()->eq('st.session_id', 's.id')) + ->where($orphanedStepsBuilder->expr()->isNull('s.id')) + ->setMaxResults($batchSize); + + $result = $orphanedStepsBuilder->executeQuery(); + $stepIds = array_map(function ($row) { + return (int)$row['id']; + }, $result->fetchAll()); + $result->closeCursor(); + + if (empty($stepIds)) { break; } + + $deleteBuilder = $this->db->getQueryBuilder(); + $batchDeleted = $deleteBuilder->delete('text_steps') + ->where($deleteBuilder->expr()->in('id', $deleteBuilder->createParameter('ids'), IQueryBuilder::PARAM_INT_ARRAY)) + ->setParameter('ids', $stepIds, IQueryBuilder::PARAM_INT_ARRAY) + ->executeStatement(); + + $deletedCount += $batchDeleted; } while ((microtime(true) - $startTime) < $maxExecutionSeconds); return $deletedCount; From a66cd75d2bdab831d37971f16405aa9e0614158a Mon Sep 17 00:00:00 2001 From: Benjamin Frueh Date: Sat, 20 Sep 2025 23:23:15 +0200 Subject: [PATCH 3/6] Add test for deleteOrphanedSteps method Signed-off-by: Benjamin Frueh --- tests/unit/Db/SessionMapperTest.php | 56 +++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/unit/Db/SessionMapperTest.php b/tests/unit/Db/SessionMapperTest.php index 310b61e957f..dc4881649b1 100644 --- a/tests/unit/Db/SessionMapperTest.php +++ b/tests/unit/Db/SessionMapperTest.php @@ -98,4 +98,60 @@ public function testDeleteInactiveWithoutStepsMultiple() { self::assertCount(0, $this->sessionMapper->findAll(1)); } + + public function testDeleteOrphanedSteps() { + $this->stepMapper->deleteAll(1); + $this->sessionMapper->deleteByDocumentId(1); + + // Create session + $session = $this->sessionMapper->insert(Session::fromParams([ + 'userId' => 'admin', + 'documentId' => 1, + 'lastContact' => time(), + 'token' => uniqid(), + 'color' => '00ff00', + ])); + + // Create steps for session + $this->stepMapper->insert(Step::fromParams([ + 'sessionId' => $session->getId(), + 'documentId' => 1, + 'data' => 'YJSDATA1', + 'version' => 1, + ])); + $this->stepMapper->insert(Step::fromParams([ + 'sessionId' => $session->getId(), + 'documentId' => 1, + 'data' => 'YJSDATA2', + 'version' => 2, + ])); + + // Create orphaned steps + $this->stepMapper->insert(Step::fromParams([ + 'sessionId' => 99999, // Non-existent session + 'documentId' => 1, + 'data' => 'ORPHANED1', + 'version' => 3, + ])); + $this->stepMapper->insert(Step::fromParams([ + 'sessionId' => 99998, // Non-existent session + 'documentId' => 1, + 'data' => 'ORPHANED2', + 'version' => 4, + ])); + + // Verify 4 steps total + $allSteps = $this->stepMapper->find(1, 0); + self::assertCount(4, $allSteps); + + // Delete orphaned steps + $deletedCount = $this->sessionMapper->deleteOrphanedSteps(); + + // Should have deleted 2 orphaned steps + self::assertEquals(2, $deletedCount); + + // Should have 2 valid steps remaining + $remainingSteps = $this->stepMapper->find(1, 0); + self::assertCount(2, $remainingSteps); + } } From 02ae309130c81688c1ca800a3cf50d8aa6e59b87 Mon Sep 17 00:00:00 2001 From: Benjamin Frueh Date: Mon, 22 Sep 2025 14:58:44 +0200 Subject: [PATCH 4/6] Changed deleteOrphanedSteps to respect document versioning and added a safety buffer Signed-off-by: Benjamin Frueh --- lib/Db/SessionMapper.php | 25 ++++++--- tests/unit/Db/SessionMapperTest.php | 86 +++++++++++++++-------------- 2 files changed, 63 insertions(+), 48 deletions(-) diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php index c4c67bb963f..ef6cd195b62 100644 --- a/lib/Db/SessionMapper.php +++ b/lib/Db/SessionMapper.php @@ -147,16 +147,25 @@ public function deleteOrphanedSteps(): int { $maxExecutionSeconds = 30; $batchSize = 1000; $deletedCount = 0; + $safetyBufferTime = time() - 86400; do { - $orphanedStepsBuilder = $this->db->getQueryBuilder(); - $orphanedStepsBuilder->select('st.id') + $orphanedStepsQb = $this->db->getQueryBuilder(); + $orphanedStepsQb->select('st.id') ->from('text_steps', 'st') - ->leftJoin('st', 'text_sessions', 's', $orphanedStepsBuilder->expr()->eq('st.session_id', 's.id')) - ->where($orphanedStepsBuilder->expr()->isNull('s.id')) + ->leftJoin('st', 'text_sessions', 's', $orphanedStepsQb->expr()->eq('st.session_id', 's.id')) + ->leftJoin('st', 'text_documents', 'd', $orphanedStepsQb->expr()->eq('st.document_id', 'd.id')) + ->where($orphanedStepsQb->expr()->isNull('s.id')) + ->andWhere($orphanedStepsQb->expr()->lt('st.timestamp', $orphanedStepsQb->createNamedParameter($safetyBufferTime))) + ->andWhere( + $orphanedStepsQb->expr()->orX( + $orphanedStepsQb->expr()->isNull('d.id'), + $orphanedStepsQb->expr()->lt('st.id', 'd.last_saved_version') + ) + ) ->setMaxResults($batchSize); - $result = $orphanedStepsBuilder->executeQuery(); + $result = $orphanedStepsQb->executeQuery(); $stepIds = array_map(function ($row) { return (int)$row['id']; }, $result->fetchAll()); @@ -166,9 +175,9 @@ public function deleteOrphanedSteps(): int { break; } - $deleteBuilder = $this->db->getQueryBuilder(); - $batchDeleted = $deleteBuilder->delete('text_steps') - ->where($deleteBuilder->expr()->in('id', $deleteBuilder->createParameter('ids'), IQueryBuilder::PARAM_INT_ARRAY)) + $deleteQb = $this->db->getQueryBuilder(); + $batchDeleted = $deleteQb->delete('text_steps') + ->where($deleteQb->expr()->in('id', $deleteQb->createParameter('ids'), IQueryBuilder::PARAM_INT_ARRAY)) ->setParameter('ids', $stepIds, IQueryBuilder::PARAM_INT_ARRAY) ->executeStatement(); diff --git a/tests/unit/Db/SessionMapperTest.php b/tests/unit/Db/SessionMapperTest.php index dc4881649b1..5af98c3f60d 100644 --- a/tests/unit/Db/SessionMapperTest.php +++ b/tests/unit/Db/SessionMapperTest.php @@ -9,12 +9,13 @@ class SessionMapperTest extends \Test\TestCase { private SessionMapper $sessionMapper; private StepMapper $stepMapper; + private DocumentMapper $documentMapper; public function setUp(): void { parent::setUp(); $this->sessionMapper = \OCP\Server::get(SessionMapper::class); $this->stepMapper = \OCP\Server::get(StepMapper::class); - + $this->documentMapper = \OCP\Server::get(DocumentMapper::class); } public function testDeleteInactiveWithoutSteps() { @@ -100,58 +101,63 @@ public function testDeleteInactiveWithoutStepsMultiple() { } public function testDeleteOrphanedSteps() { - $this->stepMapper->deleteAll(1); - $this->sessionMapper->deleteByDocumentId(1); - - // Create session - $session = $this->sessionMapper->insert(Session::fromParams([ - 'userId' => 'admin', - 'documentId' => 1, - 'lastContact' => time(), - 'token' => uniqid(), - 'color' => '00ff00', + $this->documentMapper->clearAll(); + $this->sessionMapper->clearAll(); + $this->stepMapper->clearAll(); + + $oldTimestamp = time() - 86401; + + // Create document + $document = $this->documentMapper->insert(Document::fromParams([ + 'id' => 1, + 'currentVersion' => 0, + 'lastSavedVersion' => 100, + 'lastSavedVersionTime' => time() ])); - // Create steps for session + // Create Orphaned step without document (delete) $this->stepMapper->insert(Step::fromParams([ - 'sessionId' => $session->getId(), - 'documentId' => 1, - 'data' => 'YJSDATA1', - 'version' => 1, - ])); + 'sessionId' => 99999, + 'documentId' => 99999, + 'data' => 'ORPHANED_NO_DOC', + 'version' => 1 + ])); + + // Orphaned "old" step with document and old version (delete) $this->stepMapper->insert(Step::fromParams([ - 'sessionId' => $session->getId(), - 'documentId' => 1, - 'data' => 'YJSDATA2', - 'version' => 2, + 'id' => 1, + 'sessionId' => 99999, + 'documentId' => $document->getId(), + 'data' => 'ORPHANED_OLD_VERSION', + 'timestamp' => $oldTimestamp, + 'version' => 1 ])); - // Create orphaned steps + // Orphaned "new" step with document and current version (keep) $this->stepMapper->insert(Step::fromParams([ - 'sessionId' => 99999, // Non-existent session - 'documentId' => 1, - 'data' => 'ORPHANED1', - 'version' => 3, + 'id' => 100, + 'sessionId' => 99999, + 'documentId' => $document->getId(), + 'data' => 'ORPHANED_CURRENT_VERSION', + 'version' => 2 ])); + + // Orphaned step with document and new version (keep) $this->stepMapper->insert(Step::fromParams([ - 'sessionId' => 99998, // Non-existent session - 'documentId' => 1, - 'data' => 'ORPHANED2', - 'version' => 4, + 'id' => 101, + 'sessionId' => 99999, + 'documentId' => $document->getId(), + 'data' => 'ORPHANED_NEW_VERSION', + 'timestamp' => $oldTimestamp, + 'version' => 3 ])); - // Verify 4 steps total - $allSteps = $this->stepMapper->find(1, 0); - self::assertCount(4, $allSteps); + // Verify steps for document 1 and 99999 + self::assertCount(3, $this->stepMapper->find(1, 0)); + self::assertCount(1, $this->stepMapper->find(99999, 0)); - // Delete orphaned steps + // Verify orphan delete $deletedCount = $this->sessionMapper->deleteOrphanedSteps(); - - // Should have deleted 2 orphaned steps self::assertEquals(2, $deletedCount); - - // Should have 2 valid steps remaining - $remainingSteps = $this->stepMapper->find(1, 0); - self::assertCount(2, $remainingSteps); } } From 6d9ac49deee19c86838b28780345027b6360c165 Mon Sep 17 00:00:00 2001 From: Benjamin Frueh Date: Mon, 22 Sep 2025 14:59:05 +0200 Subject: [PATCH 5/6] Changed deleteOrphanedSteps to respect document versioning and added a safety buffer Signed-off-by: Benjamin Frueh --- tests/unit/Db/SessionMapperTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/Db/SessionMapperTest.php b/tests/unit/Db/SessionMapperTest.php index 5af98c3f60d..5b093e90fff 100644 --- a/tests/unit/Db/SessionMapperTest.php +++ b/tests/unit/Db/SessionMapperTest.php @@ -118,17 +118,17 @@ public function testDeleteOrphanedSteps() { // Create Orphaned step without document (delete) $this->stepMapper->insert(Step::fromParams([ 'sessionId' => 99999, - 'documentId' => 99999, + 'documentId' => 99999, 'data' => 'ORPHANED_NO_DOC', 'version' => 1 - ])); - + ])); + // Orphaned "old" step with document and old version (delete) $this->stepMapper->insert(Step::fromParams([ 'id' => 1, 'sessionId' => 99999, 'documentId' => $document->getId(), - 'data' => 'ORPHANED_OLD_VERSION', + 'data' => 'ORPHANED_OLD_VERSION', 'timestamp' => $oldTimestamp, 'version' => 1 ])); @@ -147,7 +147,7 @@ public function testDeleteOrphanedSteps() { 'id' => 101, 'sessionId' => 99999, 'documentId' => $document->getId(), - 'data' => 'ORPHANED_NEW_VERSION', + 'data' => 'ORPHANED_NEW_VERSION', 'timestamp' => $oldTimestamp, 'version' => 3 ])); From 0c4cbb5dd9793c97742f6c2aef244baa64167f96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benjamin=20Fr=C3=BCh?= <134610227+benjaminfrueh@users.noreply.github.com> Date: Wed, 1 Oct 2025 22:54:19 +0200 Subject: [PATCH 6/6] Update lib/Db/SessionMapper.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: max-nextcloud Signed-off-by: Benjamin Früh <134610227+benjaminfrueh@users.noreply.github.com> --- lib/Db/SessionMapper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php index ef6cd195b62..b549ec5a680 100644 --- a/lib/Db/SessionMapper.php +++ b/lib/Db/SessionMapper.php @@ -153,7 +153,7 @@ public function deleteOrphanedSteps(): int { $orphanedStepsQb = $this->db->getQueryBuilder(); $orphanedStepsQb->select('st.id') ->from('text_steps', 'st') - ->leftJoin('st', 'text_sessions', 's', $orphanedStepsQb->expr()->eq('st.session_id', 's.id')) + ->leftJoin('st', 'text_sessions', 's', $orphanedStepsQb->expr()->eq('st.document_id', 's.document_id')) ->leftJoin('st', 'text_documents', 'd', $orphanedStepsQb->expr()->eq('st.document_id', 'd.id')) ->where($orphanedStepsQb->expr()->isNull('s.id')) ->andWhere($orphanedStepsQb->expr()->lt('st.timestamp', $orphanedStepsQb->createNamedParameter($safetyBufferTime)))