Skip to content

fix(dav): compare sync token retention against an absolute cutoff#60178

Open
mooons wants to merge 1 commit intonextcloud:masterfrom
mooons:fix/dav-sync-token-retention-cutoff
Open

fix(dav): compare sync token retention against an absolute cutoff#60178
mooons wants to merge 1 commit intonextcloud:masterfrom
mooons:fix/dav-sync-token-retention-cutoff

Conversation

@mooons
Copy link
Copy Markdown

@mooons mooons commented May 6, 2026

Summary

PruneOutdatedSyncTokensJob computes retention as a duration in seconds from syncTokensRetentionDays, but both DAV backends currently compare that value directly against created_at in pruneOutdatedSyncTokens().

created_at stores Unix timestamps, so this ends up using a relative duration as if it were an absolute timestamp. With the default 60-day retention, the generated condition is effectively created_at <= 5184000, which does not match normal rows.

Fix

Convert the duration into an absolute cutoff inside the backend methods before building the delete query:

$cutoff = max(0, time() - $retention);

and compare created_at against $cutoff.

This preserves the current method signatures and config semantics.

Root cause

The retention value is produced as an age duration in PruneOutdatedSyncTokensJob, but treated as an absolute timestamp in:

  • OCA\DAV\CalDAV\CalDavBackend::pruneOutdatedSyncTokens()
  • OCA\DAV\CardDAV\CardDavBackend::pruneOutdatedSyncTokens()

Impact

Without this fix, old rows in oc_calendarchanges and oc_addressbookchanges are not pruned as intended even when the background job runs.

Related: #51120

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Signed-off-by: mooons <10822203+mooons@users.noreply.github.com>
@mooons mooons force-pushed the fix/dav-sync-token-retention-cutoff branch from 8c595a8 to 938e2c1 Compare May 6, 2026 03:33
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews feature: dav feature: caldav Related to CalDAV internals feature: carddav Related to CardDAV internals bug labels May 6, 2026
@ChristophWurst
Copy link
Copy Markdown
Member

I have restored the PR template. Please fill it out.

Copy link
Copy Markdown
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Changes make sense

Thanks for the contribution!

Copy link
Copy Markdown
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

Logic looks good, but tests say no.

@SebastianKrupinski
Copy link
Copy Markdown
Contributor

Hi @mooons

Thank you for the contribution!

Could you also fix the failing tests?

There were 2 failures:

  1. OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testPruneOutdatedSyncTokens
    Failed asserting that 6 is identical to 0.

/home/runner/actions-runner/_work/server/server/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php:1403

  1. OCA\DAV\Tests\unit\CardDAV\CardDavBackendTest::testPruneOutdatedSyncTokens
    Failed asserting that 933 is identical to 0.

/home/runner/actions-runner/_work/server/server/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php:880

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug feature: caldav Related to CalDAV internals feature: carddav Related to CardDAV internals feature: dav

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants