It wasn't the first time and it won't be the last time that this
class of issue crops up :-/
This pattern:
```
match something.lock().pop() {
...
}
```
will hold the lock even after the first line has completed
and copied its data out.
This causes subtle and difficult to find issues where
the lock lifetime overlaps with some other operation.
The resoluion for this is to encapsulate the lock
operation inside a function to break the lifetime.
When `log_arf` or `log_oob` are set to true with `relay_to=false`, we
now return a 550 error response for messages that are not ARF or OOB
reports. Previously, we would return a 250 response and silently drop
the message in this case, which gave the false impression that it was
accepted for relaying.
Expand integration test to explicitly assert that the right things
are allowed/denied/relayed/parsed.
When this was added, it was to enable TSA daemon to express
and convey that a given path should be suspended.
The implementation was objectively unpleasant, turning the
fast path of egress source assignment from a simple single
iteration of the weighted round robin logic to N (where N
is the number of sources in a pool) iterations to try
and figure out if a suspension is active.
Now that we have TSA subscriptions that update the admin
suspension information in realtime, we can now simply
rely on that source of information.
This commit removes the logic associated with the deprecated
suspended flag and tidies up the code, making that fast
path a little more fast, and making the code a bit easier
to reason about.
Avoid locking self.connections multiple times, avoid grabbing
the path_config reference multiple times. Avoid building
limit structure if the connection count is within the goal.
This facilitates the removal of a slightly gross string comparison
that was performed each minute by the ready_queue maintainer,
making it slightly cheaper at the same time.
The revised locking scheme means that we can safely hold onto the
queue handle outside the loop.
Fixup the timing of handling config refreshes; previously we would
only refresh if we were completely idle for at least 60 seconds.
That wasn't the intent: this commit fixes it up so that we refresh
every 60 seconds.
Previously, we overloaded the Notify instance to mean either:
* A new message has been inserted into the ready queue
* The maintainer should wake up
The two things were often the same, but this blended responsibility
meant that we had to always use notify_waiters and couldn't use
notify_one. Not being able to notify_one was the source of flakeyness
in the maintainer_task startup situation.
This commit splits the notify instance into two; one for waking
up the maintainer, and one for waking up any Dispatcher instances
that are waiting for a message.
No need to get the current clock, lock and store the value;
we can avoid the contention and infer the time from the
wakeup event that we always send to the maintainer at
that time.
I've seen some flakey behavior in some of the low traffic integration
tests, where it looks like the maintainer isn't triggering to establish
connections.
This commit adjusts the logic in the maintainer_task to trigger
the maintainer a handful of times over the first few seconds of
its life to account for timing issues when setting up a new
ready queue on a busy server.
This commit adds two new fields to the egress source definition:
* socks5_proxy_username - string
* socks5_proxy_password - keysource
(https://docs.kumomta.com/reference/keysource/)
when present, the socks5 client will indicate that it is capable
of doing username/password authentication per
https://www.rfc-editor.org/rfc/rfc1928
The password is specified as a keysource, which allows it to
load credentials from a vault or a file in addition to a
string.
This is untested beyond compilation, so there are currently
no docs for this.
This sequence of events:
1. Messages are flowing
2. The remote site starts to return a 421 for mail at connection
time, prior to MAIL FROM, on new connections
3. The local site employs a TSA rule that suspends the corresponding
ready queue
Could result in the contents of that ready_queue getting stuck.
Here's a representation of the logs:
```
Apr 24 19:03:44 maintain SITENAME: computed ideal connection count as 3
Apr 24 19:03:44 Error in Dispatcher::run for SITENAME: connect to ResolvedAddress { name: "smtp-in.orange.fr.", addr: 80.12.26.32 } port 25 and read initial banner: Command rejected Response { code: 421, enhanced_code: None, content: "XXXXX smtp.orange.fr XXXXX Service refuse. Veuillez essayer plus tard. Service refused, please try later. OFR_999 [999]", command: None } (consecutive_connection_failures=1)
Apr 24 19:03:45 maintain SITENAME: there are now 3 connections, suspended(via config)=false, suspended(admin)=false, queue_size=691
Apr 24 19:03:45 maintain SITENAME: computed ideal connection count as 3
Apr 24 19:03:45 maintain SITENAME: there are now 3 connections, suspended(via config)=false, suspended(admin)=true, queue_size=746
... the above repeats every minute for a while ...
Apr 24 19:03:45 maintain SITENAME: computed ideal connection count as 0
Apr 24 19:13:45 maintain SITENAME: there are now 3 connections, suspended(via config)=false, suspended(admin)=true, queue_size=746
Apr 24 19:13:45 maintain SITENAME: computed ideal connection count as 0
Apr 24 19:13:45 reaping site SITENAME
Apr 24 19:23:44 maintain SITENAME: there are now 0 connections, suspended(via config)=false, suspended(admin)=false, queue_size=1
```
There are two problems:
1. The main mechanism that acts to sweep the read queue into the
scheduled queue is only triggered once we get past the initial
connection phase, causing messages to be retained in the
ready queue instead of moving back into the scheduled queue.
2. The is-reapable logic doesn't consider the number of messages
that may be in the ready queue. It was written before we had
a suspension concept and only considers that an ideal connection
count of 0 implies that there are no messages.
The result is, in combination with 1, is that we can reap the ready
queue while it contains messages and forget about them until
the server is restarted.
This commit resolves both of these issues:
1. The maintain routine will now act to sweep messages into the
corresponding scheduled queue if an admin suspension is active.
In addition, this same sweep is considered when we reap the
ready queue, just in case some other logic bug manifests in
the future with the same consequences.
2. The reap logic now also requires that the ready queue be empty.
This script is triggered as part of `make test` which is not
really a great place for it.
Its purpose is to extract the auto-generated openapi spec
from the kumod and tsa binaries and update the snapshot
that is present in the docs.
It needs kumod and tsa-daemon to have been built in debug mode
to run successfully.
It piggy-backs on `make test` on the assumption that it will
cause the person who is making changes to it to include those
spec updates in their commit/PR.
Since `make test` invokes it, the various builders may try
and fail to execute jq in the `test` step. This is mostly
harmless, but looks noisy in the logs.
The ideal situation for this would be:
* at PR time and push time: add a check that runs this script and that
fails if the specs are updated for the docs and are not part of the PR
itself. (eg: status is dirty after running it).
This way it will be visible from the CI state that something is awry.
Wez Furlong
ncai
Tom Mairs
Mike Hillyer
dependabot[bot]
Nathalie Caiandncai