trying to hone in on this, which I think is an edge case when we try to
acquire just as element(s) are expiring and producing an empty set:
```
thread 'limit::test::test_redis' panicked at crates/throttle/src/limit.rs:469:14:
called `Result::unwrap()` on an `Err` value: AnyHow(error invoking redis
lease acquisition script
key=test_redis-4e24b3a0-81f8-4cbe-abfb-16c63ee3e7cf now=1736174679.13151
expires=1736174681.13151 limit=2
uuid=346b46d2-00d6-4109-9775-a7559d484eba
Caused by:
An error was signalled by the server - ResponseError: Error running
script (call to f_2e0bdd7bbe1fa07f6cc6eebfba9ef415044c97a5):
@user_script:13: ERR value is not an integer or out of range)
```
We need to ensure that we quote the name portion of a mailbox
if it contains an @-sign in order for the resulting mailbox to
be valid.
quoting here means that the name is enclosed in quotes, but
not that the @-sign is itself quoted with a backslash, so
we have a little hard-coded special case in the string
quoting function for this.
With the recent changes to acquire_lease, the maintain() method
can now block for a couple of minutes pending acquisition of a lease.
This commit should knock it out of that wait when the system
is either shutting down, or starts running low on memory.
If we don't need to open any new connections, return early.
This just reduces the indentation level in the maintainer
method, and has not functional impact.
Previously, the lease acquire operation was a non-blocking instantaneous
operation, relying on the maintainer to check back and acquire again
later on.
Because that maintainer time interval could be relatively long, this
could lead to multiple queues hanging around not doing very much,
especially when the limit leases were under heavy contention.
This commit introduces a deadline parameter to the lease acquire method,
causing the acquire operation to wait until that deadline before giving
up with a TooManyLeases error.
For local/memory-backed mode that wait operation is provided by the
tokio Notify object; the calling task goes to sleep until awoken or the
timeout expires.
For redis-backed mode there isn't a suitable wakeup mechansim (well,
there is a redis pubsub channel, but the plumbing for this across the
various modes of redis client make integration awkward, and it would
mean that every lease acquire would ping every kumomta instance. That
traffic may not be desirable, so at this time we're not using that
mechanism), so the strategy is a simple
poll-redis-every-3-seconds-while-we-wait.
This commit encompasses the wait inside the throttle crate, and its API
fan-out, but there likely needs to be some additional smarts in the
ready queue so that that wait can be interrupted for low memory and
shutdown.
I plan to make LimitSpec the type we use for the path config
with some options that do not require a duration, so rename
this particular type in preparation for that.
No functional change here.
* add send_batch method to kafka mod
* add error messages to returned value and don't log it as error
* add kafka send_batch doc
---------
Co-authored-by: ncai <ncai@chapsvision.com>
A user reported that one of their users was having trouble
with a message that seemed to point at a multi-line Content-Disposition
header with some specific content.
This commit adds a unit test that shows that the issue was not due
solely to just this header, because it successfully parses.
Their issue is something else that is yet to be understood.
A user reported that constructing certain UTF-8 From headers
in the HTTP injection API could produce a From header that
could not be parsed by the DKIM helper when subsequently
attempting to sign the message.
The issue was that the textwrap crate will try to fill out
the wrap, preferring to break an existing word rather than
generating a new line to accommodate one when it would
produce a line that was too long.
This commit adds our own text wrapping algorithm that is
more forgiving.
This adds connection limit/throttle states to the readyq rows
in `kcli queue-summary`, alongside where we would show the
suspension state.
This makes it easier to understand when a given egress path
might be hitting connection limits.
The issue here is:
* If a connection limit was hit (eg: TooManyLeases) then the
readyq maintainer completes its work for that one wakeup
* readyq maintainer then goes to sleep until either 10 minutes
have elapsed, or a new message is moved into the ready queue
* If the system either has no new messages being injected to
that queue, or all of the messages are currently ready,
then those messages will camp out in the ready queue until
10 minutes have elapsed before we try to make another connection.
This hampers the rate of egress.
What this commit does is:
* Introduce a QueueState concept where we can indicate a kind of
"status effect" that applies to a queue. The state has some
human readable context and a time for when the effect was
most recently observed.
* Adds a `connection_limited` state to indicate when we've hit a
connection limit and a `connection_rate_throttled` state when we've
hit the max_connection_rate.
* The ready queue maintainer will reduce its wakeup interval
if it observes that connections have been limited, so that
we can wakeup sooner.
In a separate commit, an API endpoint will be added to expose
these queue states and augment the summary command output.
We had a report of a stack overflow while using this option in some
heavy load scenarios.
It's not clear that it is 100% correlated, but I'd feel better
not having recursion here so that we can rule that out.
Make the message more human readable.
Leave the site empty instead of `localhost`; no site was responsible
for the decision to expire the message, that is due to local policy.
This commit allows setting the queue config to use a unix domain socket
path:
```
protocol = {
smtp = {
mx_list = { '/some/path' },
},
},
```
and the smtp dispatcher will attempt to connect to it via a unix domain
socket.
It is anticipated that this will be used together with an egress path
config set to `use_lmtp = true`.
I've tested the ability to attempt to connect, but I don't have a
convenient implementation of SMTP/LMTP to test against here, so all I've
tested if that we can successfully fail to connect to a unix domain
socket.
refs: https://github.com/KumoCorp/kumomta/issues/267
One frustration I have with the Rust standard library types is
that while it has a SocketAddr type, that type doesn't include
unix domain addressing like the underlying unix OS does in the
underlying system type.
I understand why it is that way, it's still a bit of a PITA.
Since we want to enable the use of unix domain sockets for
outbound LMTP support, our internal addressing type needs to
be able to represent a unix domain address.
This commit introduces our own HostAddress (equivalent to IpAddr,
but not limited to IP addresses) and SocketAddress (equivalent to
SockAddr, but not limited to IP sockets) types.
The ResolvedAddress type has been cut over from IpAddr to HostAddress
and the fan out addressed.
This commit does not add support for establishing unix domain
connections, it is merely the ability to recognize/report on them.
refs: https://github.com/KumoCorp/kumomta/issues/267
When used together with an Opportunistic TLS mode, if the handshake
or subsequent EHLO fails, we will re-connect to the current host
and disable TLS.
This is implemented as a recursive solution, which I'm not totally
keen on, but the recursion is limited to a single level so it's
not so bad.
Given a provider with the following config:
```json
"match": [
{
"MXSuffix": "mta5.am0.yahoodns.net"
},
{
"MXSuffix": "mta6.am0.yahoodns.net"
},
{
"MXSuffix": "mta7.am0.yahoodns.net"
}
]
```
(Note that this configuration is not ideal because someone with
`notreallymta5.am0.yahoodns.net` in their MX records will match
this. If you were using SMTP auth for such a site, then you risk
leaking your credentials to it! We should consider adding an exact
match option for this case)
we could never match this because the logic had the inner and outer
loops swapped.
For a provider to match, all of the resolved host names must match
at least one of the MXSuffixes defined in the rule.
The flipped logic prevented that from matching.
Most of this commit is adding stuff to help trace this down
and debug it.
In particular, `resolve-queue-config` will tell you what the
effective value of the get-queue-config event is for a given
queue name, and `resolve-shaping-domain` will show you the shaping
configuration for a (bogus) source.
This option should be used with caution, and ideally only
for trusted networks.
The purpose is to absorb the latency of post-DATA processing
and hide it from the trust injector.
It defers processing that would normally happen in smtp_server_message_received
and instead will, at some (ideally) near-future time trigger an
smtp_server_message_deferred_inject event instead.
This will marginally increase your average injection latency but should
clamp your worst case injection latency much lower because the outliers
will not happen inline with the injecting client.
Expand the 3xx-is-impossible logic to include a 503 response,
which can only indicate a bad sequence of commands.
We should retry the message in that situation.
The pipelining spec allows for a server to proceed with a transaction
in which there were no valid recipients and validly enter the DATA
portion of the transaction.
I suspect that this might be responsible for the occasional protocol
synchronization issue that we've seen.
Handle that case per the recommendations in the RFC by sending
a dummy DATA in the case that some of the commands failed by
DATA succeeded.
It's bothered me for a while how verbose the debug print for
prohibited_hosts and skip_hosts is in diagnostics shared from
users.
This commit adopts a more succint map-like representation of
the cidr structure, and similar set-like representation for
the set equivalent.
The config monitor task is a long-lived task that runs
inside the same lua context for its entire lifetime.
That task obtains a reference to the current shaping data
every 30 seconds to decide whether it needs to synthesize
an update to the config epoch.
Lua's memory management cannot see the total amount of
ram consumed by the shaping data because the shaping data
is a userdata type whose ram is managed externally.
That means that lua things that this task is generally not
using many resources (~ a handful of pointers per loop iteration)
and gc doesn't kick in very aggressively.
For sites with large TSA shaping overrides, or otherwise with
very large shaping data, this can result in an accumulation
of stale shaping data in that long-lived lua context.
This commit adds an explicit gc trigger before the task
sleeps on each iteration so that we can release those references.
In 4a0a4d6a1e I fixed an issue where
the bucketing used in the timewheel could trick the throttling
event handler into thinking that the message had been explicitly
throttled by the event, when in reality it was just a 20ms discrepancy.
I overlooked that this same logic should also apply to the equivalent
logic for the singleton timer wheel, so that is what this commit does.
The goal here was to show the lowest level of stats available from the
allocator by querying jemalloc for that info, so that we can understand
the gap between what Rust sees and what any embedded C stuff might see.
```
; curl -s 'http://127.0.0.1:8000/api/admin/memory/stats'
JemallocStats {
allocated: 25654488 (25.65 MB),
active: 35651584 (35.65 MB),
metadata: 29685520 (29.69 MB),
resident: 85446656 (85.45 MB),
mapped: 364630016 (364.63 MB),
retained: 252981248 (252.98 MB),
}
RSS = 142651392 (142.65 MB)
soft limit = Some(202282060800 (202.28 GB))
hard limit = Some(269709414400 (269.71 GB))
live = CountAndSize { count: 109237 (109,237), size: 12111580 (12.11 MB) }
use kumo.enable_memory_callstack_tracking(true) to enable additional stats
```
Now that we've had a couple of users successfully running with
remember_broken_tls, let's make it the default so that we can
reduce the typical size of the TSA-generated configuration
So far, we've deferred proactively aging out expired entries from
our TTL-based LRU caches, instead relying on the next lookup operation
on the expired key to detect and replace the stale value.
Recently we have been focused on keeping the memory footprint lower.
This commit adds a periodic (every 30 seconds) background task
that will determine all cache entries that have expired, and
remove them from the cache.
The re_memory crate was good for a first pass at implementing call stack
tracking, but unfortunately, it was symbolicating the stack traces in
the allocation path which had terrible performance implications.
This commit implements our own somewhat leaner and meaner version of
this functionality that defers resolving the call stack symbols until
the top callstacks are computed.
This makes things run faster than they would have when using re_memory.
Shedding re_memory also allows us to avoid pulling in the various other
dependencies that it pulled in that were otherwise unused, which makes
me feel even better about this.
This commit also improves the output of the summarize-memory tool to
make it clearer that we are uncertain about the full extent of the
sampled allocations: the stochastic sampling only nominally samples
every 64 of the medium-sized allocations. That means that the reported
total size for those may be 1/64 of what we printed previously. We now
print the range of allocations and use a ~ character to indicate that
we're uncertain about the number of allocations that were made for those
sampled allocations.
Now that we have doubly-linked-lists of Messages for these things,
it is more efficient to take the whole list when we're doing
bulk ready queue operations, so let's do that.
The flume bounded queue doesn't eagerly allocate its full capacity
and otherwise runs fast enough for our logging that we don't notice
the difference. This reduces the "static" footprint of kumomta
in the (hopefully) common case where logging is keeping up with
the system throughput and is not approaching the limit of the
default logging backlog.
Remove unused reference to async-channel from kumo-server-runtime.
One of the things that shows up in the large allocation list
is the pre-allocated channel used by the logging infrastructure.
I'd like to adjust that to not require pre-allocation (or at least,
not require so much pre-allocation), but an easy memory reduction
win is to Box out the FBL portion of the record; incoming FBL
reports are incredibly rare compared to the bulk of the messages
processed by the system so it is reasonable to use an extra heap
allocation for those if it means we can save a nice chunk of
memory for the common case.
I've been recently troubleshooting a couple of systems with high memory
usage, and in one of them there were very large amounts of
memory being allocated to ready queues. That could be partially
mitigated by reducing `max_ready` to a more reasonable and small value,
but it is difficult to compute the right balance between large-enough
for high throughput and small-enough to keep memory usage reasonable.
This commit switches away from crossbeam's ArrayQueue, which
pre-allocates sufficient space to hold exactly `max_ready` messages for
each instantiated ready queue, and to a newly introduced MessageList,
which is an intrusive doubly-linked list.
The intrusive list, in exchange for some small additional overhead
per-Message, requires no auxilliary additional memory allocations to
track the membership of that Message in some other list.
That means that `max_ready` is no longer a pre-allocated minimum amount
of additional storage, and changes the memory overhead from
`O(number-of-queues * max_ready)` to `O(number-of-ready-messages)`,
which is typically a lot smaller. This is independent of the individual
messages metadata and bodies that are nominally associated with being in
a ready queue.
Wez Furlong
Nathalie Caiandncai
Mike Hillyer