CalDAV/CardDAV: put every method from backends that does multiple DB calls in transactions

[tcitworld]

In a lot of methods we're doing read-after-writes (for instance calling updateProperties after touching calendar objects). There's also a lot of deleting methods that do stuff sequentially which could cause trouble.
This should avoid this kind of issues.

TODO

• Fix tests

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[github-advanced-security]

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[tcitworld]

Psalm issues are existing ones.

[ChristophWurst]

Best enjoyed as https://github.com/nextcloud/server/pull/36528/files?w=1

👍 Good stuff. This might not even fix nasty database issues but improve performance a bit if changes are committed in one transaction.

[tcitworld]

I added an extra commit 27e717c to use the QueryBuilder properly in the addChange methods.

[miaulalala]

[szaimen]

@tcitworld would you mind fixing the conflicts? Then we should be able to merge this. I can also take over the rebase if you want. Thanks a lot! :)

[szaimen]

They are shown as new since the affected code is now into callbacks.

so update the baseline then in order to fix psalm?

[tcitworld]

It was a specific change in PHPDoc that didn't match the inherited class, my bad. Upstream PR at sabre-io/dav#1457

[szaimen]

/rebase

[tcitworld]

Goodbye rebase issues :)

[nickvergessen]

Goodbye rebase issues :)

Hello DB issues :(

Transaction commit failed because the transaction has been marked for rollback only.

We have quite some log entries like this in the meantime.
Stacktrace section:

      {
        "file": "…/3rdparty/sabre/dav/lib/CalDAV/Schedule/Plugin.php",
        "line": 538,
        "function": "put",
        "class": "Sabre\\CalDAV\\CalendarObject",
        "type": "->",
        "args": [
          "*** sensitive parameters replaced ***"
        ]
      },
      {
        "file": "…/apps/dav/lib/CalDAV/Schedule/Plugin.php",
        "line": 179,
        "function": "scheduleLocalDelivery",
        "class": "Sabre\\CalDAV\\Schedule\\Plugin",
        "type": "->",
        "args": [
          [
            "Sabre\\VObject\\ITip\\Message",
            "92c36c40-48fe-483b-9cff-…",
            "VEVENT",
            "REQUEST",
            1,
            "mailto:b…e@nextcloud.com",
            "*** sensitive parameters replaced ***",
            "mailto:s…s@nextcloud.com",
            "*** sensitive parameters replaced ***",
            "*** sensitive parameters replaced ***",
            [
              "Sabre\\VObject\\Component\\VCalendar",
              "*** sensitive parameters replaced ***",
              "VCALENDAR"
            ],
            false
          ]
        ]
      },
      {
        "file": "…/3rdparty/sabre/event/lib/WildcardEmitterTrait.php",
        "line": 89,
        "function": "scheduleLocalDelivery",
        "class": "OCA\\DAV\\CalDAV\\Schedule\\Plugin",
        "type": "->",
        "args": [
          [
            "Sabre\\VObject\\ITip\\Message",
            "92c36c40-48fe-483b-9cff-…",
            "VEVENT",
            "REQUEST",
            1,
            "mailto:b…e@nextcloud.com",
            "*** sensitive parameters replaced ***",
            "mailto:s…s@nextcloud.com",
            "*** sensitive parameters replaced ***",
            "*** sensitive parameters replaced ***",
            [
              "Sabre\\VObject\\Component\\VCalendar",
              "*** sensitive parameters replaced ***",
              "VCALENDAR"
            ],
            false
          ]
        ]
      },

[tcitworld]

Interesting. Sounds like we need savepoints to make sure inner transactions don't explode outer ones.
https://www.doctrine-project.org/projects/doctrine-dbal/en/current/reference/transactions.html#emulated-transaction-nesting-with-savepoints

Another suspicious issue might be #38902 (comment)

[DimanNe]

@tcitworld Do you remember why you used non retry-able atomic()?

In AppFramework/Db/TTransactional.php there is atomicRetry. Would not it make more sense to use that?

• What if PG/MySQL on a different host and there is a (temporary) network issue/disconnect?
• What if a user uses SQLite, which limits concurrency? <--- this is actually my case.

The reason I am asking is that tasks & calendars in nextcloud are essentially broken for me: almost 10-30% of events/tasks creation and/or moving fail for me in any bulk task/calendar operation (such as calendar import or move tasks from one task list to another) with the following error:

Import partially failed General error: 5 database is locked

message":"An exception occurred while executing a query: SQLSTATE[HY000]: 
General error: 5 database is locked","exception":{},"CustomMessage":"An exception occurred while executing a query:
SQLSTATE[HY000]: General error: 5 database is locked"

In the logs I have this backtrace

---

I think retry-able atomic has a flaw/issue in implementation: any retry is supposed to have some backoff policy (like truncated exponential backoff), but atomicRetry does not have one.

---

P.S.

I managed to fix my error by replacing atomic() with the following code:

protected function atomic(callable $fn, IDBConnection $db) {
    $maxRetries = 10;
    $attempts = 0;

    while (true) {
        $db->beginTransaction();
        try {
            $result = $fn();
            $db->commit();
            return $result;
        } catch (Throwable $e) {
            $db->rollBack();
            $attempts++;
            error_log("Attempt $attempts of $maxRetries failed. Exception: " . $e->getMessage() . " in " . $e->getFile() . " on line " . $e->getLine());

            if ($attempts >= $maxRetries) {
                throw $e;
            }

            $sleepMs = rand(1, 1000);
            usleep($sleepMs * 1000);
        }
    }
}

[tcitworld]

Do you remember why you used non retry-able atomic()?

In AppFramework/Db/TTransactional.php there is atomicRetry. Would not it make more sense to use that?

I have not looked into it, but this method simply didn't exist at the time (#38030 was merged two months after this one was).

[DimanNe]

You can, by the way, see how other people struggle with this issue too:

• https://help.nextcloud.com/t/import-partially-failed-general-error-5-database-is-locked/188032/1
• https://help.nextcloud.com/t/exception-an-exception-occurred-while-executing-a-query-sqlstate-hy000-general-error-5-database-is-locked/150111/2
• https://help.nextcloud.com/t/database-locked-error-recent-upgrade-to-nextcloud-19-0-2/91000
• nextCloudCmd throws error 'Sqlite exec statement error: 5 "database is locked"' desktop#3840
• export then import calender partially fails (error: 5 database is locked) calendar#5647
• Database is locked mail#4812

^^^ it seems that in most/all issues, people are using sqlite, but I have feeling that it would fail in similar way on any database, if DB and nextcloud server is on different hosts and there is a network issue (so, the root cause is not sqlite)

[DimanNe]

@tcitworld I see... Thank you for quick response.

Can you suggest what would the best way to fix the root cause of the issue (which is lack of retries with backoff policy) be?

Shall I create a new / separate issue?

[nickvergessen]

Well a retry at this place won't help if the problem is network issues, as all the other queries including the initial appconfig load will not be retried as well.
If you have concurrency issues with sqlite the fix is to use a proper DB instead.
It's even mentioned on https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html and other sections even skip sqlite from the supported lists....

[DimanNe]

@nickvergessen Thank you. Good points. I did not know about SQLite being discouraged.
(In this case, I guess, there is no point in creating any issue(s))