Skip to content
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.

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

Ensure document stays consistent with y.js state file #5477

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Mar 13, 2024

Contributes to #5476

  • fix: Clean up logic to return document state file or file content
  • fix: Set base version etag to a unique id per document creation
  • fix: Only cleanup earlier steps that haven't been persisted yet to the state file
  • fix: Keep document table entry and only cleanup when we force it

$steps = $this->documentService->getSteps($document->getId(), 0);
if (empty($steps)) {
$this->logger->debug('Empty steps, loading content for ' . $file->getId());
$content = $this->loadContent($file);
Copy link
Member Author

Choose a reason for hiding this comment

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

This might be another cause of #5420 and is not needed as we only want to load from the file content on the initial session creation

@@ -422,11 +422,12 @@ public function resetDocument(int $documentId, bool $force = false): void {

$this->unlock($documentId);

$this->stepMapper->deleteAll($documentId);
$this->stepMapper->deleteBeforeVersion($documentId, $document->getLastSavedVersion());
Copy link
Member Author

Choose a reason for hiding this comment

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

This should still be fine as a cleanup as all steps in the saved version are also persisted in the y.js state, right @max-nextcloud ?

Copy link
Member

Choose a reason for hiding this comment

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

I actually changed the logic of resetDocument() in #5481 to always delete all session data: document, sessions and steps from database as well as yjs document state file. And that will happen in the following scenarios:

  • A document gets changed from outside text and there's no unsaved steps
  • Cleanup background job runs for a stale document session (and no unsaved steps)
  • occ command text:reset is run

I wonder whether the second case should just remove sessions and steps instead.

Base automatically changed from fix/duplicate-document-init-race to main March 13, 2024 16:22
@juliushaertl juliushaertl changed the title fix/5476/document cleanup Ensure document stays consistent with y.js state file Mar 13, 2024
@juliushaertl juliushaertl added bug Something isn't working 3. to review labels Mar 13, 2024
@juliushaertl
Copy link
Member Author

Need to check the failing test:

  1. The session Api
    race conditions
    sends initial content if other session is alive but did not push any steps:

I feel we might want to adjust that to account for the changed behaviour.

@juliushaertl juliushaertl force-pushed the fix/5476/document-cleanup branch 2 times, most recently from 950b633 to e28cf61 Compare March 15, 2024 10:13
@juliushaertl juliushaertl self-assigned this Mar 15, 2024
@mejo- mejo- force-pushed the fix/5476/document-cleanup branch from e28cf61 to aee2afa Compare March 18, 2024 19:00
@mejo- mejo- force-pushed the fix/5476/document-cleanup branch from aee2afa to a63daee Compare March 20, 2024 09:17
@mejo-
Copy link
Member

mejo- commented Mar 20, 2024

Need to check the failing test:

    The session Api
    race conditions
    sends initial content if other session is alive but did not push any steps:

I feel we might want to adjust that to account for the changed behaviour.

I changed the cypress test to expect empty file content on /create requests if there's already a document session.

I also did some manual testing with removing the session connections (from oc_text_sessions) while having the document open.

@mejo- mejo- merged commit f51bce7 into main Mar 20, 2024
62 checks passed
@mejo- mejo- deleted the fix/5476/document-cleanup branch March 20, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants