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

fix: do not miss new messages while expunging the folder #5669

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Jun 4, 2024

This should fix flaky test_verified_group_vs_delete_server_after.

Fix #5201

@link2xt link2xt force-pushed the link2xt/select-with-uidvalidity branch 2 times, most recently from cdf8e30 to 3f218b0 Compare June 4, 2024 17:04
@link2xt link2xt changed the title fix: select folders using select_with_uidvalidity fix: do not miss new messages while expunging the folder Jun 4, 2024
@link2xt link2xt force-pushed the link2xt/select-with-uidvalidity branch from 3f218b0 to 003d178 Compare June 4, 2024 17:21
@link2xt
Copy link
Collaborator Author

link2xt commented Jun 4, 2024

Finally non-flaky CI, but need to think of a proper fix now.

@link2xt link2xt marked this pull request as ready for review June 4, 2024 21:27
@link2xt link2xt force-pushed the link2xt/select-with-uidvalidity branch from 01611be to e598748 Compare June 4, 2024 21:27
@link2xt
Copy link
Collaborator Author

link2xt commented Jun 4, 2024

On Windows tests failed once with "Backup provider did not start in time" message, but this is likely just because timeout is 10 seconds and GitHub Actions seem to sometimes pause/stop VMs:
https://github.com/deltachat/deltachat-core-rust/actions/runs/9374467861/job/25810847989?pr=5669
Looks like another thing we may want to fix to get non-flaky CI, but not caused by changes in this PR.
Otherwise CI is much less flaky with this change.

@@ -92,6 +96,9 @@ impl Session {
session.as_mut().set_read_timeout(Some(IMAP_TIMEOUT));
self.inner = session;

// Fetch mail once we exit IDLE.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do i correctly understand that this is just in case and not related to the fix itself? Can it be done under if self.server_sent_unsolicited_exists(context)? if so?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not just in case, without this line tests will fail.

If IDLE exits because of NewData, we want session.new_mail be true next time fetch_new_messages is called. Previously fetch_new_messages called session.select_with_uidvalidity and used its return value to decide if it should try to fetch messages. session.select_with_uidvalidity returned true if the folder was already selected, so most of the time fetch happened.

In cases other than NewData it may be not necessary, but other cases are rare so better try fetching. E.g. in case of manual interrupt it seems better to fetch, because who knows what kind of broken IMAP servers are there, closing and reopening the app should not rely on correctly working IDLE to try fetching messages.

I think IDLE response does not get into unsolicited channel, so checking for unsolicited EXISTS is not enough. Unsolicited EXISTS is an EXISTS response that you get e.g. when issuing a FETCH or STORE command.

@link2xt link2xt merged commit 380116d into main Jun 5, 2024
37 checks passed
@link2xt link2xt deleted the link2xt/select-with-uidvalidity branch June 5, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test test_verified_group_vs_delete_server_after is flaky
2 participants