Hello!
On Fri, Jul 03, 2026 at 01:09:51AM +0900, ygkat wrote:
> When soft-stop is requested, the ring forwarder applet used to close
> immediately. This dropped not only the messages still buffered in the
> ring, but also every message that in-flight streams keep producing
> while soft-stop progresses: with a ring-based log pipeline, several
> hundred milliseconds worth of access logs were lost on every reload
> (~3400 lines per reload at 3000 req/s in our tests).
First, thanks for providing a patch. I must say I've kept it low on
the review list because I find it very confusing, and every time I
approached it, my head started to hurt. I *think* that it tries to
address several issues at once, which possibly explains the difficulty
I'm facing.
> Instead, keep the forwarder flushing during soft-stop and close it once
> the data plane is drained and the ring has quiesced:
>
> - drained: no stoppable frontend connection remains, i.e. actconn
> minus the sessions of log-forward (syslog) frontends is zero.
> Syslog sessions must be excluded because they are log consumers
> which never terminate by themselves: when a ring's server points
> to a log-forward section of the same process (a common way to
> fan out logs), the forwarder's own connection would otherwise
> prevent the drain condition from ever being met and the old
> worker would hang until hard-stop-after. Non-stoppable sessions
> (master CLI) are never accounted in actconn, so plain zero is
> the right mark. Note that neither "jobs" nor "unstoppable_jobs"
> can be used here: jobs also counts listeners and appctx
> sessions, and unstoppable_jobs counts the master CLI *listener*,
> not connections, while those CLI connections never appear in
> actconn.
I partially disagree here. You seem to be making a special case of
log-forwarding because contrary to HTTP we can't easily close it,
but it is exactly the same with SSH, RDP, or whatever other raw TCP
protocol. And this is unrelated to the patch which speaks about sinks.
Here it's a different kind, it's a proxy which receives logs from
clients on one side and forwards them to servers on the other side,
so it's a service like any other one.
What would be needed instead would be to have an option in frontends
to indicate whether or not a soft-stop should close their connections
I think, because various clients are able to reconnect. The only thing
is that we need to carefully do that, as if we reset, the client will
get RST back and won't know which messages were lost. But just shutting
down TCP connections on soft-stop could be a right approach: we perform
a shut(WR) and wait up to the client's timeout (or client-fin) for the
client to close. In any case, it should be in a separate patch, and
there shouldn't be any confusing comparison with master CLI etc which
are unrelated here.
Maybe we could even have a "timeout client-stopping" that applies when
the stopping state is met, allowing to significantly shorten it. It
wouldn't force a reconnection to quickly close however, so possibly
that what we want is not a timeout but a max connection life (i.e.
forcefully shut down after that delay even for reconnections).
> - quiesced: the drain state is sampled before dispatching, so that
> the messages emitted by the last closing streams are guaranteed
> to be seen by the dispatch (streams emit their final log before
> releasing their connection, and the ring lock orders both sides).
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I didn't understand that part unfortunately. What does the ring lock
have to do with this work ?
> On top of that the forwarder only closes after the ring stayed
> empty for one extra poll period (20ms), because with chained
> sinks (ring -> log-forward -> log backend in the same process)
> the last messages may still be traversing the local relay when
> the drain is observed.
This is another complicated part to wrap up. You seem to be making a
general case of a particular deployment you're using. I'm not contesting
its value but it's super difficult to start with a solution then explain
the problem it addresses.
I seem to understand that your fundamental problem is that outgoing
traffic from rings to log servers keeps the next frontend connections
up and prevents the process from stopping. I also seem to remember that
loggers periodically try to connect when emitting logs. Thus if your
second stage frontend shuts down upon soft-stop to indicate the end is
coming, the ring will try to reconnect soon, possibly even reaching the
new process' frontend, to try to drain what it still has in its queue.
One consideration remains, ideally we should make sure we dont quit
too fast with data left in the buffer. I've seen your 20ms timer and
how it forces to ignore what's in the buffer past the 1s delay, but
what is not clear to me is how we make sure that we *do not* quit while
there are still data left there.
> Since a closing stream does not wake the forwarder, the drain state is
> re-checked every SINK_FWD_DRAIN_POLL_MS (20ms). If the ring is not
> empty but the output buffer is stuck (wedged or unreachable
> downstream), give up after SINK_FWD_WEDGE_MS (1s) without any forward
> progress so that a dead consumer cannot hold the process up to
> hard-stop-after.
The term "wedge" is not clear to me here, I think you're really just
implementing a standard timeout: no activity for that amount of time,
give up. That's already handled by "timeout server" in the ring. Maybe
you want a shorter one when stopping ?
> If the connection was never established, close
> immediately as before since there is nothing to flush to.
And if it had been established then closed ?
> Measurements on a production-like pipeline (frontend -> ring ->
> log-forward -> log backend, 3000 req/s, one reload every 10s):
>
> - log loss per reload: ~3400 lines -> 0 (15/15 reloads)
> - old worker exit: ~180ms -> ~230ms (one extra
> drain-confirmation cycle)
> - ring with two servers (two forwarder sessions): unpatched
> behaviour keeps the old worker alive until hard-stop-after on
> every reload; with this patch it exits in ~230ms
> - dead downstream: immediate close, no change (~170ms)
> - a request outliving the reload (e.g. long poll) now gets its
> final log line delivered; previously the forwarder could close
> while the last connection was still active and its log was lost
I don't contest that it brings a benefit in your use case, and I would
really like to see us make forward progress on this, however as I
explained, I'm having a very hard time tying all the pieces together
as it visibly touches several areas.
> This addresses GitHub issue #3436, which contains a minimal
> reproducer on a vanilla 3.2.20 build (268-358 lines lost per 3
> reloads at 2000 req/s with a 50ms backend, 0 without reloads).
>
> This patch is against master. For backports: 3.2 and older use
> sc_opposite(sc)->state < SC_ST_EST instead of SE_FL_APPLET_NEED_CONN
> to detect a not-yet-established connection, and co_skip() instead of
> applet_reset_input(); the rest applies as-is (a 3.2-adapted version
> was build- and load-tested the same way).
> ---
> --- a/include/haproxy/sink-t.h 2026-07-02 18:01:56.994222051 +0900
> +++ b/include/haproxy/sink-t.h 2026-07-02 18:02:30.909879796 +0900
> @@ -41,6 +41,8 @@ struct sink_forward_target {
> struct server *srv; // used server
> struct appctx *appctx; // appctx of current session
> uint last_conn; // copy of now_ms for last session establishment
> attempt
> + int wedge_exp; // soft-stop: give-up deadline while the output
> is stuck; TICK_ETERNITY/0 = unset
> + int quiet_exp; // soft-stop: close deadline while drained and
> empty; TICK_ETERNITY/0 = unset
> size_t ofs; // ring buffer reader offset
> size_t e_processed; // processed events
> struct sink *sink; // the associated sink
> --- a/src/sink.c 2026-07-02 18:01:57.018222516 +0900
> +++ b/src/sink.c 2026-07-02 18:02:30.909879796 +0900
> @@ -422,6 +422,34 @@ void sink_setup_proxy(struct proxy *px)
> sink_proxies_list = px;
> }
>
> +/* While stopping but the data plane is not drained yet, re-check that often
> + * (a stream closing does not wake the forwarder by itself). */
> +#define SINK_FWD_DRAIN_POLL_MS 20
> +/* If the downstream is wedged (we still hold messages but make no progress),
> + * give up after this long so a stuck downstream cannot hold the process up
> to
> + * "hard-stop-after". */
> +#define SINK_FWD_WEDGE_MS 1000
> +
> +/* Returns non-zero once the data plane is drained during soft-stop: no
> + * stoppable frontend connection remains, so no stream may produce new
> + * messages into the rings anymore. Sessions attached to log-forward
> + * (syslog) frontends are excluded from the count: they are log consumers,
> + * do not produce messages into local rings, and never terminate by
> + * themselves, so waiting for them would prevent the drain condition from
> + * ever being met when a ring's server points to a log-forward section of
> + * the same process. Non-stoppable sessions (e.g. the master CLI) are
> + * never accounted in actconn, so plain zero is the drained mark.
> + */
> +static int sink_dataplane_drained(void)
> +{
> + struct proxy *px;
> + int conns = HA_ATOMIC_LOAD(&actconn);
> +
> + for (px = cfg_log_forward; px; px = px->next)
> + conns -= HA_ATOMIC_LOAD(&px->feconn);
> + return conns <= 0;
> +}
> +
> static void _sink_forward_io_handler(struct appctx *appctx,
> ssize_t (*msg_handler)(void *ctx,
> struct ist v1, struct ist v2, size_t ofs, size_t len, char delim))
> {
> @@ -430,14 +458,33 @@ static void _sink_forward_io_handler(str
> struct ring *ring = sink->ctx.ring;
> size_t ofs, last_ofs;
> size_t processed;
> + int drained = 0;
> int ret = 0;
>
> if (unlikely(applet_fl_test(appctx, APPCTX_FL_EOS|APPCTX_FL_ERROR)))
> goto out;
>
> - /* if stopping was requested, close immediately */
> - if (unlikely(stopping))
> - goto soft_close;
> + /* If stopping was requested, we used to close immediately. That dropped
> + * not only the messages still buffered in the ring, but also every
> + * message that in-flight streams keep producing during the soft-stop
> + * window (e.g. access logs lost on every reload). Instead we keep the
> + * forwarder flushing and decide whether to close after the dispatch
> + * below, based on whether the data plane is drained. If the downstream
> + * connection was never established there is nothing to flush to, so we
> + * close right away.
> + *
> + * The drain state is sampled BEFORE dispatching: streams emit their
> + * final log before releasing their connection, and the ring lock taken
> + * by the dispatch synchronizes with the writer's, so once <drained> is
> + * observed here, every message emitted so far is guaranteed to be seen
> + * by this dispatch. Sampling it after dispatching would leave a window
> + * where the last messages slip in unseen and are lost on close.
> + */
> + if (unlikely(stopping)) {
> + if (se_fl_test(appctx->sedesc, SE_FL_APPLET_NEED_CONN))
> + goto soft_close;
> + drained = sink_dataplane_drained();
> + }
>
> /* if the connection is not established, inform the stream that we want
> * to be notified whenever the connection completes.
> @@ -481,6 +528,36 @@ static void _sink_forward_io_handler(str
> }
>
> if (ret) {
> + /* the ring is empty: everything available has been forwarded */
> + if (unlikely(stopping) && drained) {
> + /* Soft-stop and the data plane was already drained
> before
> + * this dispatch: no stoppable connection remains that
> could
> + * produce a new message. Messages may still be in
> flight
> + * towards this ring through a local relay though (e.g.
> + * ring -> log-forward -> log backend chained in the
> same
> + * process), so only close once the ring has stayed
> empty
> + * for one full poll period after the drain was
> observed:
> + * anything the relays still held lands in the ring in
> the
> + * meantime and resets the timer below via <processed>.
> + * This follows HAProxy's normal soft-stop drain, so the
> + * forwarder never extends the worker's lifetime beyond
> it.
> + */
> + if (!processed && tick_isset(sft->quiet_exp) &&
> + tick_is_expired(sft->quiet_exp, now_ms)) {
> + HA_SPIN_UNLOCK(SFT_LOCK, &sft->lock);
> + goto soft_close;
> + }
> + if (processed || !tick_isset(sft->quiet_exp))
> + sft->quiet_exp = tick_add(now_ms,
> SINK_FWD_DRAIN_POLL_MS);
> + sft->wedge_exp = TICK_ETERNITY;
> + }
> + else if (unlikely(stopping)) {
> + /* not drained yet: restart the quiesce confirmation
> from
> + * scratch once the drain is observed
> + */
> + sft->quiet_exp = TICK_ETERNITY;
> + sft->wedge_exp = TICK_ETERNITY;
> + }
> /* let's be woken up once new data arrive */
> MT_LIST_APPEND(&ring->waiters, &appctx->wait_entry);
> ofs = ring_tail(ring);
> @@ -492,6 +569,30 @@ static void _sink_forward_io_handler(str
> applet_have_more_data(appctx);
> } else
> applet_have_no_more_data(appctx);
> + /* Draining or confirming quiescence: the data plane may keep
> + * producing messages and a closing stream won't wake us, so
> + * re-check shortly. No wedge timeout here since the ring is
> + * empty (we are only waiting, not stuck on a busy downstream).
> + */
> + if (unlikely(stopping))
> + task_schedule(appctx->t, tick_add(now_ms,
> SINK_FWD_DRAIN_POLL_MS));
> + }
> + else if (unlikely(stopping)) {
> + /* The ring is not empty but the output is full: we still hold
> + * messages we cannot push right now. We are woken again once
> the
> + * buffer drains, but bound the wait so a wedged or unreachable
> + * downstream cannot hold the process up to "hard-stop-after":
> + * give up if no message gets forwarded for SINK_FWD_WEDGE_MS
> + * (the deadline is pushed back on every forwarded message).
> + */
> + if (processed || !tick_isset(sft->wedge_exp))
> + sft->wedge_exp = tick_add(now_ms, SINK_FWD_WEDGE_MS);
> + if (tick_is_expired(sft->wedge_exp, now_ms)) {
> + HA_SPIN_UNLOCK(SFT_LOCK, &sft->lock);
> + goto soft_close;
> + }
> + sft->quiet_exp = TICK_ETERNITY;
> + task_schedule(appctx->t, sft->wedge_exp);
> }
> HA_SPIN_UNLOCK(SFT_LOCK, &sft->lock);
Thanks,
Willy
