This commit adds unit tests to validate the behavior when delaying
a message by a negative timestamp.
The expected behavior when that message is inserted into a scheduled
queue is for it to return it back to us instead of accepting it,
because it is immediately due.
This was true for 3 out of the 4 strategies; the skiplist strategy
allowed insertion but woke up the maintainer. The net effect
of that should overall be equivalent, but it is technically
different.
This commit fixes it up so that it behaves the same as the
other strategies.
This is an regression introduced by the recent changes to support
reloading tls parameters in the esmtp listener.
We need to ensure that the counter is kept alive for the duration
of the listener so that it doesn't get immediately repead out
of the reported set of metrics.
Since we use libzstd, there may be ffi buffers associated with
any open compression streams counting against our RSS.
When memory is low, we'd like to free those if possible.
When load testing the v2 timerwheel, I was able to produce a panic
when rebinding; the panic was caused by linking an already linked
list through the associated messages.
Digging into the state management, I believe that the issue is
due to a data race between the rebind (and other similar drain
operations) and the timer wheel dispatch.
The timer wheel first locks to pop the currently due items,
unlocks, and then resolves and inspects the per-scheduled-queue
hash sets to fully de-queue the items.
The drain operation locks both the wheel and the hash set and
atomically removes its elements.
Critically, the drain removal doesn't know whether the messages that
were in its hash set either were in the linkage for the timeq
while it held the lock, or were recently removed by the timer
wheel dispatch and are pending the resolve and insertion mentioned
above.
What this commit does is make it possible to reason about that
linkage by introducing a `cancel` method on the timeq. It will
return true/false to indicate whether the message was unlinked.
To facilitate this, we need to add an index to map the message
id to the entry within the timeq, and a way to invalidate that
entry if a message is removed and re-scheduled.
Since we want the cancellation to be ostensibly an O(1) operation,
we implement it with another source of indirection; the entry slot
id. Each time an entry is inserted into a bucket slot we increment
its entry id. When we subsequently iterate over the slots we can
resolve that id to its message id and from there resolve its
entry. If that entry points back to the entry slot that we just
used to discover it, then that entry is current. Otherwise, it
is a stale entry and we can skip over (or remove) it.
Cancellation is therefore a simple O(1) removal of the msg id -> entry
mapping and doesn't need to directly intrude into the wheel buckets.
With a cancellation function in place, it means that the
per-scheduled-queue drain operation can now atomically (while both
the hash set and the wheel locks are held) reconcile the hash set
contents with the corresponding wheel contents while it cancels them: if
the cancellation fails then it means that the wheel runner has de-queued
that particular message and that it will shortly (after we release the
lock on the hash set) then try to check it against the hashset.
That means that we need to retain those particular messages in
the hash set, but drain the rest.
It feels more complex than it needs to be, and I don't really remember
why it ended up this way. I think things have simplified since the
inception of this module.
We need this to handle the multi-arch stuff that is needed to build docs on arm laptops, but our ci doesn't need to build for arm, so we adopt the multi-arch builder, but set it just to amd64 for now.
There are a few breaking changes in the API that we need to tackle
here, but they're generally fine.
Since our libunbound crate uses hickory-proto, I had to upgrade
that dep over there and reference the updated git rev here in
this commit; we don't publish that to crates.io.
The main thing that stood out in the upgrade is that the zone
file parser used by the TestResolver now seems to create
records with varying FQDN-ness. This may just be that it is
now passing data through from the underlying zone data, but
it caused a number of the SPF and DKIM test cases to fail
without canonicalizing the names to FQDN. I chose to do that
in the TestResolver rather than reviewing all the input zone
data, with the rationale being that it is least surprising
to have the test resolver fix that up than to puzzle over
records not resolving due to a missing trailing dot when
more tests are written in the future.
This commit doesn't try to take advantage of the improvements
to DNSSEC that are available in this version of hickory,
it's just upgrading to the API changes.
closes: https://github.com/KumoCorp/kumomta/pull/361
This allows pre-defining connection metadata values. When coupled with
`peer` and/or `via`, these can be done based on the corresponding
addresses associated with the session.
closes: https://github.com/KumoCorp/kumomta/issues/355
This type is similar to the prior EsmtpListenerParams type,
except that every field is optional with no specified default.
This allows us to create an initial concrete set of parameters
(that hold the defaults), then progressively layer user specified
values from different context over the top of it.
In this commit we just have a single "base" context, which is
the set of parameters defined by start_esmtp_listener.
In the future, more contexts will be factored in.
The intent is that this will hold the merged, effective set
of configuration for a listener.
Right now this is a simple subset of the existing EsmtpListenerParams,
but in a future commit the latter will be extended and allow for
contextual parameters which will need to be compiled/merged
into the ConcreteEsmtpListenerParams struct.
The motivation here is to remove tls_config from EsmtpListenerParams
to make some future configuration changes easier, so this commit
moves that simple cache out to an explicit lru ttl cache.
This has the welcome side effect of enabling periodic reloading
of the tls parameters, which in turn makes it a hands-off process
for updating certificates: we no longer require the service to
be restarted for that.
This commit removes the local cached copy of connection_gauge and
connection_denied_counter from EsmtpListenerParams. There's not
a huge benefit from caching these in the listener params, and
I'd like to make the listener configuration more flexible, so even
if the cache was useful, this spot isn't the best spot for it.
These are hooked up only for memoize at this time. No default
behavior is changed by this commit, but you can optionally
specify these parameters in order to change the behavior.
Previously, we'd use the default presentation for the Error type
reported by the idna crate. Unfortunately, that type holds no
context: it is a zero-sized empty struct type, and it chooses
to render itself as `Errors`, which isn't super helpful, but
it is hard to convey every possible error clause from the underlying
spec, so I understand why this is this way.
This commit handles that error case to present a slightly more
informative error message.
It doesn't provide context on what specifically is bad about
the input, but it at least helps to characterize that the domain
is bad, rather than imply that we don't know how to deal with
punycode at all.
The specific problem with the domain in the included test
case is most likely BIDI related per discussion in
https://github.com/servo/rust-url/issues/489
https://github.com/briansmith/ring/issues/2462 has been resolved,
so we should be buildable on amazonlinux:2 with the latest released
package.
cargo update, but also a couple of intentional bumpds to some
packages in Cargo.toml to keep things updated.
The introduction of the
`opportunistic_tls_reconnect_on_failed_handshake` option resulted in
this regression, which is because I misread the `match` statement
for this case as being only for the opportunistic case, but it
also encompasses the required case.
The issue is:
* A site has an MTA-STS policy enforcing Required tls
* The handshake with that site fails (for reasons unknown and
irrelevant)
* We would unconditionally (wrt. Required vs. Opportunistic) respect
opportunistic_tls_reconnect_on_failed_handshake and re-queue the
current address for the next connection attempt
* Ordinarily, opportunistic_tls_reconnect_on_failed_handshake +
the remembered broken state would cause that next attempt to
downgrade to clear text, but MTA-STS forces the policy to
Require
* Goto step 2 (modulated by connection rate throttling)
The fix is simply to only apply
opportunistic_tls_reconnect_on_failed_handshake when the policy
is actually opportunistic.
We were using a fairly tight limit of 16 messages in the channel
that buffers the effects of changing bounces/suspensions from
any websocket-connected-clients.
A busy server could hit that limit fairly easily, resulting
in a `channel lagged by NUMBER` error that drops the websocket,
causing the client to need to reconnect and resync.
This commit resolves that by making the buffer a much more healthy size.
We were deduping just by rule_hash, but each of these tables has
additional required fields as part of the primary key.
The result was that, for sites with a lot of bounces/suspensions
triggered by the same rules across a related set of sources,
the full set of bounces and suspensions would not be correctly
reported as part of a websocket push.
Previously, we'd build a little map of existing rules and check
things off against it, but that doesn't have the best scaling
properties, because we need to copy the full set of bounces/suspensions
into lua and check against that.
Since the underlying bounce/suspension tracking stuff already
guarantees that inserting a record with the same criteria will
replace another, this checking in lua-space was redundant and
wasteful, so we can just remove it.
This commit also adds some debug logging to show how many
records are being processed.
While doing a torture test with a very large combination of
the number of domains, campaigns and tenants (effectively
unbounded in any meaningful time period), using the smart sink
to generate bounces and suspensions, I found that the overhead
of tracking the set of bounces and suspensions by scheduled
queue was too high: each bounce lookup was O(number-of-bounces),
and when you can have 100k admin bounces configured, that is
not good for the miss case, which, despite the volume of admin
bounce records, is still the dominant case.
This commit introduces a helper map/multi-index type that
maintains indices for the common tuples:
* (domain, campaign, tenant),
* (domain, tenant)
* (domain)
* other
then uses an appropriate index based on the current context.
That reduces the the lookup cost to effectively constant time (3 x O(1)
lookups), unless there are admin bounces registered in the "other"
bucket, in which case there is an additional O(number-of-other) lookups.
sqlite doesn't have a native async interface, and instead will
use traditional OS-level mutexes to ensure thread safety.
Using those when under contention in a tokio scheduler thread
can lead to blocking of the tokio scheduler threads, which can
prevent timely delivery of data via websockets, or timely
processing of incoming log records.
This commit fixes up the sqlite access points to use tokio's
spawn_blocking function to move that style of mutex acquisition to a
more suitable context.
We'll wait up to 3s at a time for however many mesages are available
to extract from the tsa daemon websocket, then process the results
in batches.
This avoids the potential for geometric complexity if there is a run of
subscription updates happening around the same time.
Wez Furlong
Ryan Bonnell