Skip to content

feat: allow adding second transport #7348

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 20, 2025
Merged

feat: allow adding second transport #7348

merged 1 commit into from
Nov 20, 2025

Conversation

@link2xt
Copy link
Collaborator

@link2xt link2xt commented Oct 24, 2025 •
edited
Loading

Tracking issue: #7357

Based on (already merged) #7424, #7413, #7406, #7392

Adding second transport is only allowed with bcc_self disabled for now, synchronization of transports will be a separate PR. This PR is fine for adding multiple transports on a single-device setup already and switching between them.

Connectivity view will display something, but extending it for multi-transport is out of scope for this PR, this is in the tracking issue.

  • move resync_request from Context to Imap (already merged in refactor(imap): move resync request from Context to Imap #7406)
  • add transport_id column to imap table
  • add transport_id column to imap_sync table
  • set mvbox_move to 0 when the first transport is created, even if it is not chatmail
  • do not allow to create a second transport if mvbox_move or only_fetch_mvbox is enabled
  • do not allow to enable mvbox_move or only_fetch_mvbox if more than one transport is configured
  • test that current ConfiguredAddr transport cannot be deleted
  • do not allow to set ConfiguredAddr to the address that does not belong to any transport
  • run IMAP connections for all transports at once when I/O is started. For SMTP we only run selected transport, this works already
  • test that vCards contain the current primary address
  • Fix migration 73 to use direct SQL queries
  • restart I/O automatically when configured_addr is set
  • delete corresponding imap and imap_sync entries when the transport is deleted
  • fix flaky legacy examples/test_examples.py::test_echo_quit_plugin (disabled it, want to finish this PR instead of debugging legacy CFFI test)
  • do not create imap_sync entries for transport_id 0 during configuration
  • clean up SMTP queue when configured_addr is changed or keep multiple SMTP connections until SMTP queue for non-primary transports is empty

@link2xt link2xt self-assigned this Oct 24, 2025
@link2xt link2xt force-pushed the link2xt/multitransport branch 4 times, most recently from 9ca5385 to d3a4eee Compare October 26, 2025 09:12
@link2xt link2xt mentioned this pull request Oct 26, 2025
Open
6 tasks
@link2xt link2xt force-pushed the link2xt/multitransport branch 4 times, most recently from a288b16 to d538e8f Compare October 31, 2025 18:03
@link2xt link2xt mentioned this pull request Oct 31, 2025
Open
3 tasks
@link2xt link2xt force-pushed the link2xt/multitransport branch 7 times, most recently from b866d3e to 5d66931 Compare November 3, 2025 14:33
@link2xt link2xt changed the base branch from main to link2xt/transport-module November 3, 2025 14:39
@link2xt link2xt force-pushed the link2xt/multitransport branch from 5d66931 to cbdae22 Compare November 3, 2025 14:40
@link2xt link2xt force-pushed the link2xt/transport-module branch from e2a6579 to 098c134 Compare November 3, 2025 18:14
@link2xt link2xt force-pushed the link2xt/multitransport branch from cbdae22 to 91c3957 Compare November 3, 2025 18:14
Base automatically changed from link2xt/transport-module to main November 3, 2025 18:58
@link2xt link2xt force-pushed the link2xt/multitransport branch from 91c3957 to 933903f Compare November 3, 2025 19:05
@link2xt link2xt changed the base branch from main to link2xt/query_and_then November 3, 2025 19:13
Base automatically changed from link2xt/query_and_then to main November 3, 2025 23:02
@link2xt link2xt force-pushed the link2xt/multitransport branch from 590cf06 to 22cdb13 Compare November 3, 2025 23:04
@link2xt link2xt force-pushed the link2xt/multitransport branch 2 times, most recently from 5f75474 to a2416ed Compare November 3, 2025 23:45
@link2xt link2xt force-pushed the link2xt/multitransport branch 9 times, most recently from 6151afc to c97938e Compare November 13, 2025 15:18
@link2xt
Copy link
Collaborator Author

link2xt commented Nov 13, 2025

This is now ready for review.

It is a minimal support for multi-transport, allowing to add transports and switch between them while being connected to all of them over IMAP. It does not support synchronization, so adding transports should only be allowed when multi-device is not enabled ("bcc_self" is false).

Most important to review is the SQL migration, everything else can be changed later.

@link2xt link2xt requested review from Hocuri and iequidoo November 13, 2025 15:47
@link2xt link2xt mentioned this pull request Nov 14, 2025
Merged
iequidoo
iequidoo reviewed Nov 14, 2025
View reviewed changes
@link2xt link2xt force-pushed the link2xt/multitransport branch 4 times, most recently from 1bc4b3c to 12fb31c Compare November 17, 2025 16:15
@link2xt link2xt mentioned this pull request Nov 18, 2025
Draft
iequidoo
iequidoo reviewed Nov 18, 2025
View reviewed changes
src/config.rs
// The messages in the queue have a different
// From address so we cannot send them over
// the new SMTP transport.
transaction.execute("DELETE FROM smtp", ())?;
Copy link
Collaborator

@iequidoo iequidoo Nov 18, 2025

Choose a reason for hiding this comment

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

It seems that messages should be re-sent over the new transport. Maybe just calling chat::resend_msgs() works, it doesn't require that messages should be from the same chat

Copy link
Collaborator Author

@link2xt link2xt Nov 20, 2025

Choose a reason for hiding this comment

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

I moved this as a TODO item to the tracking issue. Ideally we need to add another column for SMTP queue so each transport has its queue and messages are sent over secondary transports as well, but not in this PR.

Copy link
Collaborator

@iequidoo iequidoo Nov 20, 2025

Choose a reason for hiding this comment

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

Then the messages should get OutFailed, otherwise they'll hang forever in OutPending i guess

Copy link
Collaborator Author

@link2xt link2xt Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it should be fixed for real by not removing them from the queue before multi-transport is marked as non-experimental. Some messages are not visible, so dropping them is a temporary solution anyway. For now it will indeed hang in the "pending" state forever (or until the user resends the message manually).

@iequidoo iequidoo mentioned this pull request Nov 18, 2025
Closed
Hocuri
Hocuri reviewed Nov 19, 2025
View reviewed changes
Hocuri
Hocuri approved these changes Nov 19, 2025
View reviewed changes
Copy link
Collaborator

@Hocuri Hocuri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM once comments are addressed; I'll approve already so that this can be merged quickly.

As far as I can see, the most important open point is whether we need to add transport_id to more sql tables. (edit: apparently, that's not necessary)

@link2xt link2xt force-pushed the link2xt/multitransport branch from 12fb31c to 14cc597 Compare November 19, 2025 23:16
@link2xt link2xt force-pushed the link2xt/multitransport branch from 5c22a53 to d4e183d Compare November 20, 2025 15:30
@link2xt
Copy link
Collaborator Author

link2xt commented Nov 20, 2025

I squashed everything into a single commit now. Will merge it after reviewing it myself again and once CI passes.

@link2xt link2xt merged commit be3e202 into main Nov 20, 2025
29 checks passed
@link2xt link2xt deleted the link2xt/multitransport branch November 20, 2025 15:51
@iequidoo
Copy link
Collaborator

iequidoo commented Nov 20, 2025 •
edited
Loading

This could go into the commit message:

Adding second transport is only allowed with bcc_self disabled for now, synchronization of transports will be a separate PR. This PR is fine for adding multiple transports on a single-device setup already and switching between them.

Connectivity view will display something, but extending it for multi-transport is out of scope for this PR, this is in the tracking issue.

And the issue reference as well

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

Reviewers

@r10s r10s r10s left review comments

@Hocuri Hocuri Hocuri approved these changes

@iequidoo iequidoo iequidoo left review comments

Copilot code review Copilot Copilot left review comments

Assignees

@link2xt link2xt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@link2xt @iequidoo @r10s @Hocuri