Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement periodic full sync job to repair cache inconsistencies #10038

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Aug 21, 2024

Run a background job once a week without any Horde cache to forcibly disable QRESYNC support. This should reconcile the UID list in our database.

I also added a button to the mailbox action menu.

grafik

Close #9168

@st3iny st3iny self-assigned this Aug 21, 2024
@st3iny st3iny changed the title fix(imap): persist vanished messages immediately on EXAMINE commands feat: implement periodic full sync job to repair cache inconsistencies Aug 21, 2024
@st3iny
Copy link
Member Author

st3iny commented Aug 26, 2024

TODO

  • Add an entry to a the context menu to run this action from the frontend

@st3iny st3iny force-pushed the fix/imap/persist-deleted-messages branch 3 times, most recently from 44c5315 to 06fa8a8 Compare August 30, 2024 19:12
@st3iny st3iny force-pushed the feat/periodic-full-sync branch 2 times, most recently from 71d22f5 to ca3d6ce Compare September 10, 2024 08:04
@st3iny st3iny force-pushed the fix/imap/persist-deleted-messages branch from 67a12d7 to c7dff93 Compare September 11, 2024 19:10
@st3iny st3iny force-pushed the fix/imap/persist-deleted-messages branch from 33cc44d to 3b577fe Compare September 23, 2024 06:49
Base automatically changed from fix/imap/persist-deleted-messages to main September 23, 2024 08:16
@st3iny st3iny force-pushed the feat/periodic-full-sync branch 2 times, most recently from 89dd295 to 7f3418f Compare September 24, 2024 11:04
@st3iny st3iny marked this pull request as ready for review September 25, 2024 09:59
@kesselb
Copy link
Contributor

kesselb commented Sep 25, 2024

The menu looks like a candidate for a design review ;)

Even without clear cache, because that's hidden unless debug enabled, it's cluttered.
And "subscribed" is a bit lost 🤣

image

Do you think we should limit the repair command for a mailbox to run once per 10 minutes or so? (we could use the UserRateLimit attribute for it).

It was rather quick even with my larger mailbox.

@st3iny
Copy link
Member Author

st3iny commented Sep 30, 2024

Do you think we should limit the repair command for a mailbox to run once per 10 minutes or so? (we could use the UserRateLimit attribute for it).

Yeah, that is probably a good idea. Although I wasn't sure how to reflect this in the frontend. Disable the button while the rate limit is active via setTimeout()?

It was rather quick even with my larger mailbox.

I was able to confirm this using a very large test mailbox as well. The request itself should not cost that much processing time. It's comparable to a regular sync and we don't have a rate limit there.


PS: I fully agree about the menu desperately requiring a redesign 😆

@st3iny
Copy link
Member Author

st3iny commented Sep 30, 2024

/backport to stable4.0

@kesselb
Copy link
Contributor

kesselb commented Sep 30, 2024

Yeah, that is probably a good idea. Although I wasn't sure how to reflect this in the frontend. Disable the button while the rate limit is active via setTimeout()?

With the UserRateLimit attribute, the request will fail with a different status code. It would be nice to show a toast in that case, "hello, please wait 60 minutes before the next repair run". I don't think it's necessary to check that right away when building the menu or disable the button.

Would it be possible to disable the button or show a loading animation while the request is running?

@st3iny
Copy link
Member Author

st3iny commented Sep 30, 2024

I'll add a rate limit annotation and add a toast.

Would it be possible to disable the button or show a loading animation while the request is running?

The button is disabled during the repair. I didn't add a loading animation though.

@st3iny
Copy link
Member Author

st3iny commented Sep 30, 2024

Hmm, on second thought, this will limit repairing multiple distinct mailboxes. I ajdusted the rate limit to 3 attempts every 10 minutes.

@kesselb
Copy link
Contributor

kesselb commented Sep 30, 2024

Hmm, on second thought, this will limit repairing multiple distinct mailboxes. I ajdusted the rate limit to 3 attempts every 10 minutes.

Sure! 10 attempts in 10 minutes is also okay. It's just to have a minimal protection against abusing the feature.

@kesselb
Copy link
Contributor

kesselb commented Sep 30, 2024

I was able to confirm this using a very large test mailbox as well. The request itself should not cost that much processing time. It's comparable to a regular sync and we don't have a rate limit there.

Sorry, I overread that part. Also, okay for me to not apply a user rate limit if you think we don't need it.

@st3iny
Copy link
Member Author

st3iny commented Sep 30, 2024

I'll add a more lax rate limit and then this is done.

Copy link
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.

:shipit:

lib/BackgroundJob/RepairSyncJob.php Outdated Show resolved Hide resolved
@pabloeisenhut
Copy link

Sorry to ask, but is it intended that everyone has the ability to approve changes?

@st3iny
Copy link
Member Author

st3iny commented Oct 1, 2024

Sorry to ask, but is it intended that everyone has the ability to approve changes?

Yes, that is a GitHub feature. However, we have branch protections in place that enforce approvals by developers with write permissions before merging. That is why your check mark is shown greyed out.

@pabloeisenhut
Copy link

Sorry to ask, but is it intended that everyone has the ability to approve changes?

Yes, that is a GitHub feature. However, we have branch protections in place that enforce approvals by developers with write permissions before merging. That is why your check mark is shown greyed out.

Oh, perfect, I was wondering, thanks for the clarification! :)

@st3iny st3iny force-pushed the feat/periodic-full-sync branch from e4298a3 to c0bed86 Compare October 1, 2024 19:01
@st3iny st3iny enabled auto-merge October 1, 2024 19:02
@st3iny st3iny merged commit 6e3779a into main Oct 1, 2024
34 of 35 checks passed
@st3iny st3iny deleted the feat/periodic-full-sync branch October 1, 2024 20:01
@st3iny
Copy link
Member Author

st3iny commented Oct 2, 2024

/backport to stable3.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

Externally deleted emails show up in the inbox and cannot be deleted
4 participants