use relay's maxsmtprecipients metadata for smtp chunking, to remove another is_chatmail use

[hpk42]

Relays since chatmail/relay#960 and released mid may with relay-1.11 set maxsmtprcipients imap metadata to 1000. I manually verified that nine.testrun.org, mailchat.pl and a few others have it set already.

this PR also moves recipient chunking to smtp loop where it belongs because it's transport specific.

[link2xt]

Comment here should be adjusted at the same time because it still says that chunks are stored separately:

core/src/receive_imf.rs

Lines 549 to 568 in e2b67d2

	// It sometimes happens that a slow server (usually a classical email server)
	// receives a message via SMTP,
	// but then the connection to the server dies before it sends the OK response.
	// In order to handle this case, we delete the SMTP send jobs if we receive our own message via IMAP.
	//
	// Now, messages with long recipient lists are split into multiple SMTP jobs.
	// In this case, we only want to delete the SMTP job that was sent to self
	// because this is the only chunk we can be sure was sent out.
	let self_addr = context.get_primary_self_addr().await?;
	context
	.sql
	.execute(
	"DELETE FROM smtp \
	WHERE rfc724_mid=?1 AND (recipients LIKE ?2 OR recipients LIKE ('% ' || ?2))",
	(rfc724_mid_orig, &self_addr),
	)
	.await?;
	if !msg_has_pending_smtp_job(context, msg_id).await? {
	msg_id.set_delivered(context).await?;
	}

The logic is likely not working at all because self-sent message may be missed during prefetch, could likely be removed (see #8366).

[link2xt]

self here is a Context, so this metadata is not per-transport and is just the metadata fetched from some server rather than the sending transport.

[hpk42]

uh right. so i guess it's better to have a context.max_smtp_rcpt_to as a transport_id->max_rcpts map and write/read from that? I would like to avoid adding a column to transport sql table (had this in an earlier revision but needs a migration). However, context.max_smtp_rcpt_to could lead to smtp reading it before imap metadata reading filled it for the transport, but worst case it uses 50 recipients chunking for this message. If that trade-off is not acceptable, then i don't see another solution than using a new column for transport table.

sidenote: conceptually it would probably make sense to have context.metadata be transport dependent completely. But that would blow up the PR i am afraid, and is better done in a separate PR.

[link2xt]

Separate map should be fine.

update_metadata has access to self.transport_id, so it can check if the limit was already fetched, but this fetching should be before the early return that is for the case when metadata is already fetched. Ideally the whole metadata should be turned into per-transport map, but then all the code that uses metadata, e.g. iroh relays, needs an update to pick arbitrary (e.g. the first one, should be fine for BTreeMap) transport.

[hpk42]

turned out that making metadata transport dependent actually was much easier than i feared, and even saves a few LOCs in the end. Previously i thought we have to do more to get a metadata value, but indeed just first one is fine. It was last writer and now it's whatever btreemap yields first, both pretty random. b1aefc4

[hpk42]

core/src/receive_imf.rs

Lines 549 to 568 in e2b67d2

	// It sometimes happens that a slow server (usually a classical email server)
	// receives a message via SMTP,
	// but then the connection to the server dies before it sends the OK response.
	// In order to handle this case, we delete the SMTP send jobs if we receive our own message via IMAP.
	//
	// Now, messages with long recipient lists are split into multiple SMTP jobs.
	// In this case, we only want to delete the SMTP job that was sent to self
	// because this is the only chunk we can be sure was sent out.
	let self_addr = context.get_primary_self_addr().await?;
	context
	.sql
	.execute(
	"DELETE FROM smtp \
	WHERE rfc724_mid=?1 AND (recipients LIKE ?2 OR recipients LIKE ('% ' || ?2))",
	(rfc724_mid_orig, &self_addr),
	)
	.await?;
	if !msg_has_pending_smtp_job(context, msg_id).await? {
	msg_id.set_delivered(context).await?;
	}

The logic is likely not working at all because self-sent message may be missed during prefetch, could likely be removed (see #8366).

i fixed the comment. Also tried removing the deletion logic but that makes test_mark_message_as_delivered_only_after_sent_out_fully and test_resend_after_ndn fail, so at least the tests exercise it.

In reality, smtp-lost-ok case should be rare and sending twice is deduplicated at receiver sides. Cleartext usage (where receivers may not deduplicate) does not really matter for this edge case.
But still think the code/test removal is better done in a separate PR.

[link2xt]

TODO for the future: this may collect all ICE servers, maybe using multiple TURN servers at once makes the calls more reliable.

[link2xt]

This if can be moved above, before t.prepare(), to skip preparing the statement that is not used afterwards.

[link2xt]

Type can be changed to return u32 to avoid conversions (especially as which is not well-defined). It's anyway bad idea to use usize in the logic, the rare exception is buffer sizes. usize is accidentally 32 bit on old android phones that are still supported while everything is tested on 64-bit platforms.

[link2xt]

Better use u32::try_from, see https://github.com/chatmail/core/blob/main/STYLE.md#do-not-use-into-try_into-or-parse