fix(dav): Add retention time to sync token cleanup #44075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChristophWurst
added
bug
2. developing
|
@tcitworld does this makes sense? |
9 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tcitworld
approved these changes
Mar 8, 2024
| @@ -52,9 +52,10 @@ public function __construct(ITimeFactory $timeFactory, CalDavBackend $calDavBack | |||
|
|
|||
| public function run($argument) { | |||
| $limit = max(1, (int) $this->config->getAppValue(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000')); | |||
| $retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetention', '60')) * 24 * 3600; | |||
There was a problem hiding this comment.
Suggested change
| $retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetention', '60')) * 24 * 3600; | |
| $retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetentionDays', '60')) * 24 * 3600; |
Make the name explicit the unit
|
/backport to stable28 |
|
/backport to stable27 |
|
/backport to stable26 |
| @@ -52,9 +52,10 @@ | |||
|
|
|||
| public function run($argument) { | |||
| $limit = max(1, (int) $this->config->getAppValue(Application::APP_ID, 'totalNumberOfSyncTokensToKeep', '10000')); | |||
| $retention = max(7, (int) $this->config->getAppValue(Application::APP_ID, 'syncTokensRetentionDays', '60')) * 24 * 3600; | |||
Check notice
Code scanning / Psalm
DeprecatedMethod Note
There was a problem hiding this comment.
Thanks, Psalmy. I want to backport
There was a problem hiding this comment.
backport ease vs. future tech debt
⚖️
The never ending balancing act...
There was a problem hiding this comment.
I'll send a PR when this is in
ChristophWurst
added
3. to review
ChristophWurst
added
4. to release
8 tasks
ChristophWurst
force-pushed
the
fix/dav/sync-token-retention-time
branch
from
3054011
to
4ddd76c
Compare
|
|
ChristophWurst
added
2. developing
Merged
ChristophWurst
force-pushed
the
fix/dav/sync-token-retention-time
branch
from
3d68b2d
to
f3e295f
Compare
|
Fixed existing tests and added two new assertions that assert all data is kept when the retention threshold is timestamp=0. |
ChristophWurst
added
4. to release
|
There is still some errors in CI |
ChristophWurst
force-pushed
the
fix/dav/sync-token-retention-time
branch
from
f3e295f
to
77e2c60
Compare
|
Addressed. Now CardDAV has the same new test to assert the logic keeps historic data :) |
ChristophWurst
force-pushed
the
fix/dav/sync-token-retention-time
branch
2 times, most recently
from
f84412a
to
dd35045
Compare
tcitworld
reviewed
Mar 21, 2024
apps/dav/tests/unit/BackgroundJob/PruneOutdatedSyncTokensJobTest.php
Outdated
Show resolved
Hide resolved
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
force-pushed
the
fix/dav/sync-token-retention-time
branch
from
dd35045
to
3dea99f
Compare
This was referenced Mar 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is a refinement of #31064. We keep 10k sync changes by default. The larger the user base, the fewer sync tokens can be stored per user on average. E.g. if you have an installation of 5k active calendar users, each of them can only keep 2 sync changes. This leads to synchronization problems with Thunderbird and similar.
This change adds timestamp tracking to changes and only deletes changes older than 60 days. The retention period is configurable. 7 days is the minimum.
TODO
Checklist