Add test coverage for source_selection_rate and tidy up how we reflect
being throttled due to source_selection_rate; in the first cut
we made it look like the ready queue was full, but now we're a
bit more deliberate:
* If any of the candidate queues were full, we report that the
ready queue was full so that the message can be retried again
"soon".
* If we hit a source_selection_rate throttle, we use the throttle
delay to adjust the min delay period and if we run out of candidate
sources, we'll have the same sort of return value as we would
for a suspension with a definite duration; the message will
log that transient failure in the same way with the same sort
of retry schedule. I've revised the wording in that case to be
"no sources for SITE pool=`POOL` are eligible for selection at this time"
(the important part being `at this time`) to suggest at the
transient nature of the unavailability.
* If we exhaust all sources for other reasons, we'll treat this
as the NoSources case. In practice, this should mean that the
source has zero weight. We'll now report "no sources available for SITE
pool=`POOL`" in this case instead of the more precise but wordy
"no non-zero-weighted sources available for SITE" that we used
prior to this commit.
This switches to the two-phase insert, we first reserve,
the redeem the reservation to perform the insert.
This has no immediate benefit, but will allow the use
of a just-in-time check against eg: a warm-up throttle
for the source.
Previously, we'd pick a source whether it had room for the new
message or not, then generate a TransientFailure when we subsequent
figure out that it is full.
This commit will try to deliver through one of the other possible
sources instead of delaying the message.
This is a small refactor to perform the actual message insertion
as part of the selection logic. It will enable more advanced
actions in the future, and it simplifies the logic in the
queue.rs side of things a little.
The struct that performs our weighted round robin will be expanded
in a later commit, this adjusts its name to better reflect its
purpose, rather than its current implementation.
This field only accepts IP addresses; it is an error to use
a DNS name with it.
The sources helper was swallowing the helpful part of the context
about the issue with that field.
This commit ensures that we do surface it.
If there is a runtime error when first loading the policy,
we would claim that we failed to parse it, which is misleading.
Adjust the wording to "Error evaluating policy" rather than
"Failed to parse policy".
We can't use the very latest version of this right now because
there are some build changes in that version that break on
amazonlinux:2.
I think 0.17.12 will probably be ok, but I stand ready to figure
out next steps if this is also having trouble on amazonlinux:2
refs: https://github.com/briansmith/ring/issues/2462
closes: https://github.com/KumoCorp/kumomta/pull/353
While auditing Answer::as_txt usage as a follow up from the recent
SPF fix, I noticed a TODO in the dkim code (which we forked from
another implementation) to support processing multiple TXT
records.
This commit implements the necessary tweaks to extract multiple
signatures and attempt to verify them against the incoming message.
The issue here was essentially a data fidelity issue around
TXT record representation.
A DNS TXT record can be composed from multiple strings, and a domain can
return multiple TXT records, so there is some nesting.
The SPF RFC says:
```
3.3. Multiple Strings in a Single DNS Record
As defined in [RFC1035], Sections 3.3 and 3.3.14, a single text DNS
record can be composed of more than one string. If a published
record contains multiple character-strings, then the record MUST be
treated as if those strings are concatenated together without adding
spaces. For example:
IN TXT "v=spf1 .... first" "second string..."
is equivalent to:
IN TXT "v=spf1 .... firstsecond string..."
TXT records containing multiple strings are useful in constructing
records that would exceed the 255-octet maximum length of a
character-string within a single TXT record.
```
so the SPF logic was dutifully joining records together around the
empty string.
Howerver, if you look at `dig yahoo.com txt` you'll see a bunch
of non-SPF records:
```
yahoo.com. 1800 IN TXT "google-site-verification=Z3-Vh6zqUMgybVH4wQl1GxKSKN7JE13kyCyeZ3TZZ-I"
yahoo.com. 1800 IN TXT "v=spf1 redirect=_spf.mail.yahoo.com"
yahoo.com. 1800 IN TXT "Zoom=13284637"
yahoo.com. 1800 IN TXT "edb3bff2c0d64622a9b2250438277a59"
```
these were getting joined together and producing a bogus input.
Obviously we should not join the results from the txt lookup
together like that, but then why would the RFC make a point
of talking about joining stuff together, and where should
that logic live?
Our `Answer::as_txt` implementation was doing some joining
of its own and it turned out that it was concatenating across
the outer layer of the aforementioned TXT record nesting.
This commit fixes that up and adds some test coverage.
There should be no functional change here; this commit refactors
how we pre-declare CallbackSignatures with lua. The rationale is:
* It is important to correct declare single vs. multiple implementation
event handler types before any lua code is run; we've had a couple
of issues in the past where some part of this pre-registration
was messed up.
* Adding a new signature requires declaring it in a global, and
remembering to add its registration to the right place
* Declaring signatures is a bit boilerplatey and makes it hard
grok the purpose of the event handler arguments at a glance
The declare_event! macro defined in this commit makes it a bit
more readable to declare these event types and automatically
wires up the registration to the correct spot, improving the
ergonomics significantly.
A downside of this additional layer of macro stuff that it
requires increasing the rustc recursion limit.
Let's have it abort any pending waiters: we only call clear
when memory is low so we want all those things to unwind
in a timely fashion without getting blocked.
This commit adds a declare_cache! macro that serves a couple of
purposes:
* Make it slightly less boilerplatey to define a cache
* Register them explicitly as a list of predefined caches, so that
we can raise an error if lua code would conflict via memoize
* Enable a future change that allows reconfiguring the cache parameters
for those predefined caches
Note: if you are using clang as a linker via RUSTFLAGS/.make-env,
the linkme crate that is employed for the registry may result
in linker errors. The solution that I have in my `.make-env` is
to add `-Clink-arg=-Wl,-z,nostart-stop-gc` to RUSTFLAGS so that
its sections are not removed too soon in the build process:
```
; cat .make-env
ROCKSDB_LIB_DIR=/usr/lib/x86_64-linux-gnu
RUSTFLAGS=-C linker=clang -C link-arg=-fuse-ld=lld --cfg tokio_unstable -Clink-arg=-Wl,-z,nostart-stop-gc
```
I worry that 1 minute might be too short to be universally good,
particularly as a change in behavior from an earlier build: going from
no timeout to 1 minute could be super surprising and un-welcome.
Let's bump this up to 5 minutes which more closely matches the
default for DATA in the base SMTP RFC.
After making spool stores respect the data processing deadline, in soak
testing it was interesting to see that the outliers were not actually
due to spooling, but instead due to latency spikes around delays
fetching data for the various lua hooks for queue and shaping helpers to
enforce things like provider level shaping.
This commit passes the optional deadline through to the queue insertion
logic, populating it only for the initial ingress and not for the
requeue or spool enumeration cases.
The application of conditional timeouts pushes the size of the compiled
Futures over a stack-friendly threshold which (thankfully) showed up
immediately in integration tests. The remedy for those is to sprinkle
Box::pin around various futures to encourage them to live in the heap
instead of on the stack.
I'm a bit concerned about this, as the stack usage is workload
dependent: I'm not sure that I've covered off enough of these
to ensure that we won't see stack overflows in the future.
If you are running into a stack overflow please report it!
Note that you can mitigate a stack overflow by exporting
`RUST_MIN_STACK=33554432` into the environment of kumod to
set a larger default size.
This commit catches the timeout error and converts it into
a `451 4.4.5 data_processing_timeout exceeded (insert)` instead
of letting it bubble up as a 421 technical difficulties response
that would also have closed the connection.
I've been having trouble with moka's eviction policy leading
to some unwanted duplicate work, especially when the system
is under load and the latency is higher.
This commit swaps it out in favor of a relatively simple
implementation on top of dashmap; that gives us a decent
concurrent base and better respects the concurrency
limit and LRU around the contentious insertion case.
For LRU, since dashmap is a concurrent data structure, it
is ~impossible to use the classic doubly linked LRU approach
safely.
The strategy used here is a proabalistic sampling LRU using a technique
similar to that used in redis.
We use atomics to tag read and write entries with a monotonic counter.
Entries with smaller counter numbers are least-recent than entries with
a larger number.
During eviction we take a random sample of 10 entries from the cache
map, remove any that have expired due to TTL, and if we still need space
after that, we'll pick up to half of those to evict. That guarantees
that we don't pick the most recent entry of the sample, and on aggregate
this should be a reasonable approximation of the "true" LRU.
In the background, every 30s, a maintenance task will remove expired
entries from the caches. This is primarily to reduce memory utilization
when the cache is otherwise idle. When a cache is hot, expiration is
performed as part of the eviction logic described above.
This commit fixes up a few edge cases:
* The recent switch to moka for lruttl adopted an LFU eviction
policy which could choose not to take a recently updated value
in some situations, which is undesirable for expensive cache
population functions. Explicitly configure it to use classic
LRU for its eviction policy.
* The semaphore based thundering herd control in memoize had
a very hard to pin down edge case where we could end up
blocking for an extended period of time. Switch to the
`.get_or_try_insert_with_freshness_status` method that moka
enabled in our lruttl module
* That didn't eliminate the edge case, but rather limited the
scope of its negative effects. There is still an edge case,
but now it results in serial duplicate calls to the cache
population function, which is wasteful but not as egregious
as the extended period of blocking.
I've opened https://github.com/moka-rs/moka/discussions/502
with moka to see what is going wrong there
* Adopt dashmap for the map of caches, to reduce hypothetical
contention across multiple caches. This is a micro optimization.
* Improved tracing of lruttl and memoize
Under high concurrency, a backlog of store operations can build
up, especially if rocksdb isn't tuned for the workload.
In order to work together with the data_processing_timeout of the smtp
server, we'd like to be able to cancel store operations that are past
the deadline, but the spool operations are not guaranteed to be cancel
safe because both the rocksdb and the local disk implementations can
ultimately call into spawn_blocking to perform the critical portion of
the work, and that is not cancel safe.
This means that we cannot simply apply timeout_at to the store
operation: while it may appear to the caller to have been cancelled at
the appropriate time, the actual work storing the spool may be in
progress and continue to store the message to the spool, which could
lead to us thinking that we are accountable for the message when we next
restart, even though we're presumably about to tell the sender that we
did not take the message.
To deal with this situation more safely, we pass the deadline through
to the underlying operation and have it internally decide whether
it should continue.
In practice, this means that the rocksdb store operation will apply
timeout_at to the semaphore operation it does if
`limit_concurrent_stores` has been configured fr that spool. Otherwise
the deadline is ignored.
The implementation of this is imperfect in a number of ways. A more
robust solution would be to employ something like
<https://docs.rs/cancel-safe-futures/0.1.5/cancel_safe_futures/coop_cancel/index.html>,
but that is a more invasive change and one that is a bit difficult to
wrangle across the spool trait definition in its current form.
If `limit_concurrent_stores` is not in use, this commit doesn't
change the behavior of the spool store operation.
This commit addresses a tricky interaction with lua's garbage collection
and cancellation safety of async lua call handling.
In Rust, when a future is cancelled it is simply not called any more,
and any resources it owns will be released when it is dropped.
In mlua's Rust bindings, async calls are futures that are wrapped
up into state whose ownership is managed in the lua context
stack, which in turn has managed via its own garbage collection
process.
In KumoMTA, we implement a pool of Lua contexts which we keep
around for reuse. Our logic will return our LuaConfig object
back to this pool automatically when it is dropped.
In a situation where tokio timeouts or select! are used with
a future that transitively contains a lua async call and that
call is cancelled, that can leave async locks/semaphores owned
in the un-collected objects in the lua stack.
For async locks/semaphores that were in the locked state, this
can leave them locked until the owning lua context is expired
from the pool, either due to age, capacity or use count being hit.
That's bad because it can block certain processing flows for 5 minutes
(the default `kumo.set_max_lua_context_age`).
What this commit does is switch away from automatically returning
the LuaConfig to the pool on Drop. In that case the lua context
is destroy on Drop, immediately allowing destructors to release
any/all owned resources.
We do still want to benefit from the pool, so the bulk of
this commit is changing the consumers of load_config to
explicitly call `config.put()` to put the object back into
the pool, but only in the success case.
This does penalize flows that grab a config and then
`kumo.reject` as their final status. At the present time, the
majority of kumomta deployments are very much focused on (relatively)
trusted senders and an outbound use case, so this isn't a terrible
situation for the time being.
This is a lock free concurrent map, providing a micro optimization
in the queue resolution code path when concurrent actors are
resolving different queues.
Previously, this code implemented work deduplication through the
use of a simple Notify object to signal waiters after the primary
task had completed.
I'd like to apply a timeout to the resolve function but the simplistic
signalling mechanism will be prone to leaving pending waiters stalled
if the task that is responsible for filling out the slot is simply
never polled again once the timeout is reached--it doesn't behave
well in the cancellation case.
This commit switches to a more robust signalling mechanism that
uses a Semaphore to ensure that all waiters have a chance to wake
up and respond to a cancelled task on the same queue name.
While in here, I put in a negative cache so that in a persistent
error scenario, we're not just subjecting the queue of waiters
to re-attempt something that is likely to fail in a similar
time frame.
This is an internal timeout that applies across the `process_data`
method, which encapsulates the span of time between receiving the
last part of the DATA payload and responding to it.
The intent here is that we will return a timeout 4xx response to
the injecting client if we take too long.
In most cases we can do this at the point that the deadline is
reached, but there are some cases where we cannot cancel the
ongoing work and instead will detect the timeout after the deadline
has been reached.
We will stop short of putting the message into the queue subsystem
in that case, and attempt to unwind the message from the spool
and emit appropriate log records.
The intent here is *not* to set a firm deadline on *responding* to DATA,
but rather to ensure that we don't assume responsibility for relaying
the message *if* the deadline was exceeded. It is still possible for
processing to exceed the deadline.
This behavior is more valuable than it may seem at first glance because
a client that times out waiting for the server to respond has no way to
know if the send was successful, and if its strategy is to retry the
message AND the server accepted responsibility for it, then the result
will be a duplicate delivery each time the timeout is experienced.
Wez Furlong
Ryan Bonnell