Fix relation handling in syncroton
ClosedPublic
Actions

Authored by mollekopf on Aug 29 2024, 8:34 AM.

Details

Reviewers

machniak

Group Reviewers

Syncroton Developers

Commits

rSaba440b5876d: Adjust cleanup to preserve the "before" timestamp.
rS32a5bc2feee9: Deal with collisions in the cache
rS6ed687a23516: Avoid resetting the cache on read
rS35f1f3a7f785: Checking for empty leads to duplicate inserts when the new set of
rSb5e8c08eb076: Fix race condition in relation state handling
rS71838700603e: RelationsTest

Summary

We had a variety of issues in the syncroton handling code:

Race conditions where sometimes we wouldn't detect changes or attempt duplicate inserts
Invalid handling of an empty set of members in the cache (use of empty instead of isset)
Invalid resetting of the cache before reading, resulting in duplicate inserts

The solution is somewhat messy because we really shouldn't be using timestamps for this, but rather attach the information to the syncstate, but this seems to fix most of the issues and gets the test to reliably pass.

Supersedes https://git.kolab.org/D4896

Diff Detail

Repository

rS syncroton

Branch

dev/relationfixes

Lint

Lint Skipped

Unit

No Test Coverage

Build Status

Buildable 48831
Build 18336: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Aug 29 2024, 8:34 AM

mollekopf created this revision.

Harbormaster completed remote builds in B48831: Diff 14043.Aug 29 2024, 8:34 AM

mollekopf retitled this revision from RelationsTest to Fix relation handling in syncroton.Aug 29 2024, 8:38 AM

mollekopf edited the summary of this revision. (Show Details)

mollekopf added a reviewer: Syncroton Developers.

Another less confusing option for the cache would be to use "before" and "after" in the cache instead of timestamps, because we really want to cache two separate things that a timestamp happens to separate usually (but not always).

lib/kolab_sync_storage.php
1987	I need to adjust the comment or the query (I experimented with both variants <> and <)

Adjust cleanup to preserve the "before" timestamp.

Harbormaster completed remote builds in B48834: Diff 14046.Aug 29 2024, 10:44 AM

mollekopf retitled this revision from RelationsTest to Fix relation handling in syncroton.Aug 29 2024, 11:01 AM

mollekopf edited the summary of this revision. (Show Details)

machniak requested changes to this revision.Aug 29 2024, 12:03 PM

machniak subscribed.

machniak added inline comments.

lib/kolab_sync_storage.php
59	You have to change that in kolab_sync_storage_kolab4 too.
1574	You could store `$since->format('Y-m-d H:i:s')` in a variable and use it for code clarity.
1950	Redundant empty line.
tests/Sync/Sync/RelationsTest.php
157	The cache is an implementation detail we shouldn't be asserting. I guess it can stay for now.

This revision now requires changes to proceed.Aug 29 2024, 12:03 PM

Adressed comments

Harbormaster completed remote builds in B48837: Diff 14049.Aug 29 2024, 12:24 PM

machniak accepted this revision.Aug 29 2024, 12:25 PM

This revision is now accepted and ready to land.Aug 29 2024, 12:25 PM

mollekopf added inline comments.Aug 29 2024, 12:26 PM

lib/kolab_sync_storage.php
59	good catch
tests/Sync/Sync/RelationsTest.php
157	This is not a generic activesync test (as opposed to the kolab4 infrastructure test), so I think it's fine. We specifically want to make sure that the cache cleanup actually does something.