File might not be saved if close is happening too fast

[juliusknorr]

Cypress test showing the issue:

text/cypress/e2e/directediting.spec.js

Lines 56 to 78 in f7596d2

	it('Open an existing file, edit and close it', () => {
	createDirectEditingLink(randUser, 'empty.md')
	.then((token) => {
	cy.logout()
	cy.visit(token)
	})
	const closeRequestAlias = 'closeRequest'
	cy.intercept({ method: 'POST', url: '**/session/close' }).as(closeRequestAlias)
	cy.getContent()
	.type('# This is a headline')
	.type('{enter}')
	.type('Some text')
	.type('{enter}')
	cy.get('button.icon-close').click()
	cy.wait(`@${closeRequestAlias}`).then(() => {
	cy.getFileContent(randUser, 'empty.md').then((content) => {
	// FIXME: This currently fails due to the save not happening fast enough
	// The best would be if we always send the markdown at least on close and perform a save if the content changed
	// expect(content).to.equal('# This is a headline\n\nSome text');
	})
	})

There is a sync request issued before close but that may not trigger the save due to the short timespan.

We might want to always send the markdown content with the close request to save if nothing changed.

[max-nextcloud]

While working on #3543 I ran into a failing cypress test where the sync before the close returned a 500.

Here's the trace

{
  "reqId": "8G5JkCKBhPxixNXWzeys",
  "level": 3,
  "time": "2023-01-05T11:07:27+00:00",
  "remoteAddr": "192.168.26.5",
  "user": "gnimxqwtq",
  "app": "index",
  "method": "POST",
  "url": "/index.php/apps/text/session/sync",
  "message": "Call to a member function getUserId() on bool in file '/var/www/html/apps-extra/text/lib/Controller/SessionController.php' line 116",
  "userAgent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/108.0.0.0 Safari/537.36",
  "version": "26.0.0.1",
  "exception": {
    "Exception": "Exception",
    "Message": "Call to a member function getUserId() on bool in file '/var/www/html/apps-extra/text/lib/Controller/SessionController.php' line 116",
    "Code": 0,
    "Trace": [
      {
        "file": "/var/www/html/lib/private/AppFramework/App.php",
        "line": 172,
        "function": "dispatch",
        "class": "OC\\AppFramework\\Http\\Dispatcher",
        "type": "->",
        "args": [
          {
            "__class__": "OCA\\Text\\Controller\\SessionController"
          },
          "sync"
        ]
      },
      {
        "file": "/var/www/html/lib/private/Route/Router.php",
        "line": 298,
        "function": "main",
        "class": "OC\\AppFramework\\App",
        "type": "::",
        "args": [
          "OCA\\Text\\Controller\\SessionController",
          "sync",
          {
            "__class__": "OC\\AppFramework\\DependencyInjection\\DIContainer"
          },
          [
            "text.Session.sync"
          ]
        ]
      },
      {
        "file": "/var/www/html/lib/base.php",
        "line": 1045,
        "function": "match",
        "class": "OC\\Route\\Router",
        "type": "->",
        "args": [
          "/apps/text/session/sync"
        ]
      },
      {
        "file": "/var/www/html/index.php",
        "line": 36,
        "function": "handleRequest",
        "class": "OC",
        "type": "::",
        "args": []
      }
    ],
    "File": "/var/www/html/lib/private/AppFramework/Http/Dispatcher.php",
    "Line": 165,
    "Previous": {
      "Exception": "Error",
      "Message": "Call to a member function getUserId() on bool",
      "Code": 0,
      "Trace": [
        {
          "file": "/var/www/html/apps-extra/text/lib/Controller/SessionController.php",
          "line": 91,
          "function": "loginSessionUser",
          "class": "OCA\\Text\\Controller\\SessionController",
          "type": "->",
          "args": [
            "*** sensitive parameters replaced ***"
          ]
        },
        {
          "file": "/var/www/html/lib/private/AppFramework/Http/Dispatcher.php",
          "line": 225,
          "function": "sync",
          "class": "OCA\\Text\\Controller\\SessionController",
          "type": "->",
          "args": [
            "*** sensitive parameters replaced ***",
            "*** sensitive parameters replaced ***",
            "*** sensitive parameters replaced ***",
            1,
            "## Hello world",
            false,
            false
          ]
        },
        {
          "file": "/var/www/html/lib/private/AppFramework/Http/Dispatcher.php",
          "line": 133,
          "function": "executeController",
          "class": "OC\\AppFramework\\Http\\Dispatcher",
          "type": "->",
          "args": [
            {
              "__class__": "OCA\\Text\\Controller\\SessionController"
            },
            "sync"
          ]
        },
        {
          "file": "/var/www/html/lib/private/AppFramework/App.php",
          "line": 172,
          "function": "dispatch",
          "class": "OC\\AppFramework\\Http\\Dispatcher",
          "type": "->",
          "args": [
            {
              "__class__": "OCA\\Text\\Controller\\SessionController"
            },
            "sync"
          ]
        },
        {
          "file": "/var/www/html/lib/private/Route/Router.php",
          "line": 298,
          "function": "main",
          "class": "OC\\AppFramework\\App",
          "type": "::",
          "args": [
            "OCA\\Text\\Controller\\SessionController",
            "sync",
            {
              "__class__": "OC\\AppFramework\\DependencyInjection\\DIContainer"
            },
            [
              "text.Session.sync"
            ]
          ]
        },
        {
          "file": "/var/www/html/lib/base.php",
          "line": 1045,
          "function": "match",
          "class": "OC\\Route\\Router",
          "type": "->",
          "args": [
            "/apps/text/session/sync"
          ]
        },
        {
          "file": "/var/www/html/index.php",
          "line": 36,
          "function": "handleRequest",
          "class": "OC",
          "type": "::",
          "args": []
        }
      ],
      "File": "/var/www/html/apps-extra/text/lib/Controller/SessionController.php",
      "Line": 116
    },
    "CustomMessage": "--"
  }
}

• Requests on the client side: 500-when-closing-minimal.har.zip

[max-nextcloud]

I can still reproduce this with the cypress test given. The current workaround in the test is to wait for two sync requests.

If we remove this the test will fail:

diff --git a/cypress/e2e/directediting.spec.js b/cypress/e2e/directediting.spec.js
index 35e233364..532823a4f 100644
--- a/cypress/e2e/directediting.spec.js
+++ b/cypress/e2e/directediting.spec.js
@@ -61,16 +61,11 @@ describe('direct editing', function() {
                        })
                const closeRequestAlias = 'closeRequest'
                cy.intercept({ method: 'POST', url: '**/session/close' }).as(closeRequestAlias)
-               cy.intercept({ method: 'POST', url: '**/apps/text/session/sync' }).as('sync')
                cy.getContent().type('# This is a headline')
                cy.getContent().type('{enter}')
                cy.getContent().type('Some text')
                cy.getContent().type('{enter}')
 
-               // ensure we have received our own steps
-               cy.wait('@sync', { timeout: 7000 })
-               cy.wait('@sync', { timeout: 7000 })
-
                cy.get('button.icon-close').click()
                cy.wait(`@${closeRequestAlias}`).then(() => {
                        cy.getFileContent('empty.md').then((content) => {

[max-nextcloud]

There's a push happening after the close. But that ends up in a 403 - forbidden as the session was closed. However the save would happen with a sync not a push anyway.

There's also a sync right before the close. It even has the proper autosave content and the manualSave flag set.

I think this fails because its version is not up to date. So basically the steps have not been synced and therefore the server ignores the autosave content. I think we should just save anyway even if the client is not up to date. Losing data because a close does not save is much more likely than loosing it because a file was saved without the latest changes synced and the other user who made the other changes does not save afterwards.