This fixes an issue where the cache being scoped globally could allow
the same IP/domain combination to appear to be satisfied by an earlier
authenticated session with the same IP/domain combination, for a period
of 60 seconds (the default TTL that we used for that cache).
This commit moves the cache to be smaller and more focused in scope;
now each session remembers the last few domains (bounded, to avoid
a trivial DoS by a malicious client) made on it.
closes: https://github.com/KumoCorp/kumomta/issues/320
I was halfway through adding special purpose options for this,
but I realized that skip_hosts already exists for this function
and is much more flexible.
Add an example to the docs to show how it can be done.
The same technique can be used to skip using IPv4 if that is
desired (despite being impractical with the current state
of SMTP on ipv6), but using `0.0.0.0/0` as an entry in the
skip_hosts list.
closes: https://github.com/KumoCorp/kumomta/issues/317
Previously, we would only trigger the requeue_message event in
situations where we were actively working on talking to the destination.
That left issues such as persistently NXDOMAIN destinations as being
unable to be caught and handled by the requeue_message event, which is
an issue for sites that want to fail out messages from the queue that
have bogus domains before they reach max_age.
This commit replaces all but one of the force_into_delayed calls with
requeue_message, and makes the call out to the event unconditional
(rather than dependent upon whether we were incrementing retries or
not).
The only case now that doesn't cause requeue_message to fire is when the
ready queue is full. The rationale is that that is a transient local
resource issue (rather than some external factor to which we need to
react), and that is likely to be a hot event when it triggers, so we
don't want to add CPU pressure with calling out to the requeue event for
them.
refs: https://github.com/KumoCorp/kumomta/issues/319
The timerwheel achieves its cheap insertion and removal by
bucketing events with a slight loss in precision.
It is possible for messages to be popped because they are due "now", but
the precise now value for any given message might still be a small
number (tens) of milliseconds in the future.
Separately from this, there is logic that checks to see if the various
throttling related events have delayed any messages and will reinsert
those messages into the scheduled queue.
That logic can be falsely triggered by the slight imprecision and
cause a message to miss its true scheduling window. I've observed
this case manifest in the retry_schedule test case.
This commit deals with this case by ensuring that we wait until all
of the due messages are really due; in practice this is either 0ns
or ~20ms.
Explicitly spawn via the main tokio runtime when starting the http
listener for TSA, so that the tasks spawned by axum are also spawned
there.
Previously, it would stick to whichever localset thread was used for
initializing the lua context, constraining all http request decoding and
dispatching to that same thread.
With this change in place, in the NOP (stale log record) case I can get
180k qps, this is likely as fast as it will go on this particular
machine, as it is 100% CPU bound on all cores.
With a current log record I can get 90k qps. In that case, the sqlite
performance is the limiting factor and the cores are around 70-80%
saturated.
For comparison, the rate I get with 2024.11.22-17fafff1 is 27k qps, so
this series of commits overall represents a 3.3x improvement in
performance.
Note that the http server spawning change may also benefit kumod's http
injection API. I deliberately left that as it was because it is worth
being deliberate about benchmarking that before just shipping the same
change--kumod has an http injection pool to explicitly manage
performance and care should be taken to evaluate the impact on that.
The original thesis of this commit was: if we are trying to clear a
backlog of log records whose matching rules + the original start time of
the record have an expiration that is already in the past, then we
should skip doing anything with the database and we'll have a faster
throughput.
It turned out to expose an edge case in the batch channel that caused it
to slow down quite significantly in that case.
This commit adds a relatively simplistic batching channel that will
batch up to a pre-defined size / latency before notifying waiting
threads to wake up and grab a batch.
The performance is comparable to the high-level throughput I observed
with batch-channel in the best case.
I can't seem to saturate more than a single core with tsa-daemon in my
load testing; that is close to 62k qps which is more than I think we
should ever see from kumomta, but it bothers me that we don't appear to
be able to go faster. I suspect that something in axum's dispatching
and extractor (eg: the json parsing!) is the bottleneck there, and that
it could potentially be removed by not using the convenient axum Json
extractor.
When load testing with 1000 connections and a low nfile ulimit,
I saw that it was possible to get into a state where the majority
of requests would fail due to having too many open files.
This commit addresses that by having a single config loading
thread, which reduces the number of actual files needed to load
the config, which then publishes a shared copy of the resulting
config. That allows the actual worker threads to grab the most
recent copy and use that, without trying to load it.
If there is a transient issue with reloading the config, the
last published config is retained and used until the next
load attempt succeeds and updates it.
On my test system, running `k6 run tsa-torture.js` (included in this
commit) could achieve ~26,000 qps.
With the changes in this commit I see ~60,000 qps.
The gains are realized by introducing batching and transactions in the
publish code path, and allowing sqlite to avoid a synchronous flush.
Turning down the tracing::info! that was logging when rules
were matched made a non-trivial impact on throughput;
35k qps with info!, 60k qps with it at the current debug!, so I've
put it at debug.
This commit introduces a processor pool with n-cpus as
the number of threads as the way that we ingest log
records.
The ingestion is done via a batch-channel which will
allow the potential for automatic batching into larger
transactions in a later commit.
This commit causes publish requests to return as soon
as the record is queued into the channel, rather than
once the record has been processed. This means that
there is now a window of opportunity where, if tsa-daemon
were to panic or be killed, the associated records will
be forgotten. There is a maximum of 128k records that
can be in the queue at one time, which puts an upper
bound on this.
In practice, it is not critical if records are lost
in this way: it is not significantly different from
the time window where a node might continue to send
mail before the effect of an automation kicks in.
The net effect of this is that the submission latency
is now much lower while the system remains un-saturated,
but will then block additional submissions until the
backlog is cleared.
Previously we'd use the Debug impl of ResolvedAddress, which rendered
like `ResolvedAddress { name: "some.host.", addr: "10.0.0.1" }`, which
is a bit heavyweight when it shows up in a log where we're indicating
that none of the hosts could be connected.
This commit adds a Display impl that renders that same struct in a
more compact form: `some.host./10.0.0.1`.
At reception time, we've supported using the special queue name
"null" as a way to indicate that a message should be accepted
but then silently dropped.
Since that was built, we've added a couple of ways through which
a message can be assigned to a different queue after reception.
In a few situations we've had users that want to assign those
messages to the null queue to indicate that they should be dropped.
That has "worked" but not for the right reasons: setting the queue
name to "null" is equivalent to setting the routing domain to the
domain named "null", which doesn't resolve to a valid MX and that
will eventually cause messages that were queued there to expire
out of the system.
This commit introduces a special case for a queue named "null"
that configures it with a special "Null" delivery handler.
Similar to the maildir handler, this one will immediately
handle ready messages without instantiating a corresponding
ready queue for them.
In this case, handling them means simply deleting them with
no other logging.
We don't log here in order to be consistent with the reception-time
behavior of not logging. It might be desirable to add some
configuration around this in the future, but for the moment, the primary
way that messages might end up assigned to a "null" queue is via a
rebind operation, and that will log an `AdminRebind` record that can be
used to capture that change in responsibility.
If logging a bounce is desired, it is recommended to use kumo.reject to
bounce the message during the rebind event instead of assigning it to
the null queue.
The primary purpose of this is so that I can do this:
```
ROCKSDB_LIB_DIR=/usr/lib/x86_64-linux-gnu
```
on my ubuntu system and cut down on rocksdb related build times.
This should help to age out messages in pathological cases where
the bulk of the messages are not actually being attempted, but
rather being requeued due to excessive connection failures or
other bulk queue operations.
Really, I just wanted the debug output in CI to be less overwhelming,
but while I'm in here, might as well switch to parking_lot.
This commit switches from the derived Debug impl to just serializing
the metadata as json, which is much more compact.
previously, we'd generate a trace_id only in the SmtpDispatcher
for the tracing subsystem.
We want to capture a session id in the logs, so the first step
is to formalize this field.
refs: https://github.com/KumoCorp/kumomta/issues/316
Following on from b64219bab4, if we
do encounter a 354 response to a message send, it must indicate
a protocol synchronization issue, so let's treat the associated
message as it had transiently failed and explicitly break the
connection (like we do for a 421).
I did consider making the associated message immediately eligible
to retry, but I worry that if that particular message is a poison
pill we might get stuck in a short cycle of retries for it.
I don't think it is likely for this particular message to be the
poison pill, because it is most likely the victim of an earlier
issue on the connection, but I still want to be cautious here.
The outcome here is still not ideal, but it is better than marking
the associated message as a permanent failure.
This makes it easier to set a default for remember_broken_tls
without it causing issues for sites that have a transient blip
with TLS, when TLS is set to required for those sites.
We've seen evidence in some logs that it is possible to end up
with a protocol synchronization issue where we get back a 354
response (which can only occur from the DATA verb) when we think
we just sent RCPT TO.
It is unclear where the error is coming from as the logic in
the smtp client is already fairly defensive and emits speculative
RSET calls for each message.
What I suspect might be happening is that we somehow encounter
a transient issue where we don't assertively close the connection.
We do have what I considered to be fairly exhaustive handling of
the various error cases in the smtp_dispatcher deliver_message
function, so it's not obvious where the issue might be coming
from.
So, what does this commit do?
* Refactor the smtp client logic code that is responsible for sending
the request side of things so that we just have two cases, rather
than the previous handful
* If the write fails either due to a timeout or because of some
other transport error, we now expliticly break the client by
clearing out its socket field. This will prevent continued
use of the client.
* in smtp_dispatcher, if we end a send with a broken client and
haven't already propagated an error up to the caller, we will
raise an error. That will cause the session to try the next
host in the connection plan or just tear down the session
depending on the overall state of that session.
Previously, we added `batch_size` which acts as a maximum batch
size; this was the simplest possible way to achieve batching
with a constrained upper bound with no additional latency.
This commit introduces a minimum batch size and a maximum latency.
The behavior for delivery handlers that configure min_batch_size > 1
is that once we've started a batch, we'll allow for up to
max_batch_latency time to pass to accumulate more messages into
the batch before we just go ahead and send it.
If you wanted to batch say 5,000-10,000 messages per batch or whatever
you get in a 10 second time period you would set:
* min_batch_size = 5000
* batch_size = 10000
* max_batch_latency = "10s"
Note that the default idle_timeout in the product is "5s" so if you want
max_batch_latency to be effective you will also need to increase the
idle_timeout in your shaping configuration/egress path config for that
handler.
Wez Furlong
Dirkjan Ochtman