Hi,
On Fri, Jul 31, 2026 at 03:03:03PM +0900, ygkat wrote:
> Hi Willy,
>
> Thanks for taking the time, and sorry about the headache. You are right
> that the patch addresses several problems at once, and I made it worse
> by starting from the solution instead of the problem. Let me restart
> from the problem.
Thanks for explaining with more context.
> But first, a bug report on my own patch.
No worries, this happens.
> 2. The problem, with no solution attached
> =========================================
>
> Two distinct losses happen at soft-stop with a ring-based log pipeline:
>
> (a) whatever is still in the ring when soft-stop is requested is
> dropped, because the forwarder closes on the first "stopping"
> test;
>
> (b) whatever the still-running streams produce during the soft-stop
> window is dropped as well, because by then no forwarder is left
> to read the ring.
Exactly!
> In our tests (b) dominates. At 3000 req/s the old worker needs about
> 180 ms to finish its last streams, and every log line emitted in that
> window is lost: around 3400 lines per reload.
I'm not surprised at all, since streams are delayed by application
response time, while usually local TCP loggers tend to send at wire
speed and to have large fat pipes.
> Issue #3436 has a
> reproducer on a vanilla build -- 268 to 358 lines lost per 3 reloads at
> 2000 req/s, and 0 without reloads.
In fact even at 1 req/s with a 1s application response time you could
get 100% losses since rings are closed faster.
> The requirement is just this: the last stream to finish must still get
> its log line out. Everything else in my patch was me working around
> what makes that hard, which brings me to the next point.
I agree with the goal. In fact, I've long had a problem with the
delayed communication with other agents around. TCP logs have this
problem, just like peers. Peers addressed it via an increment of
the global jobs variable, which counts the number of non-observable
active jobs that must not be interrupted. I'm not certain all
situations address this properly (e.g. if a stick-table pushes an
update into an empty update list, and wakes the peer task up, it's
possible that the soft-stop landing between the two will result in
an immediate close).
For a long time, proxies have been supporting a "grace" parameter which
was almost never used, and which was there to say "I'm staying alive for
this long before stopping". It was causing some trouble (keeping ports
bound etc) which is in part why it was later removed. But this is exactly
what we'd want in the sink in my opinion: a way to say "I'm ignoring the
soft-stop during this time, continuing my forwarding activities, including
reconnect attempts if required, and waiting for new logs even if my
buffer is empty". Because here the goal would definitely be to get any
stream or connection's messages.
However there is no way to know that no stream nor connection will have
anything else to report anymore. And actconn is not a reliable indicator
since it can be incremented when you relay to a local forwarder.
Thus my feeling is that we just want to say "I'm OK with waiting that
long after soft-stop for the following events to finish":
- in-flight connections being accepted and possibly emitting logs
- existing connections emitting logs
- existing connections accepting streams that might produce a log
- existing streams to finish and emit logs
- flushing of local ring buffer to the servers, even if that
includes a reconnection attempt.
I really feel that beyond time there's no indicator here that we should
stop waiting. The state of the ring buffer doesn't indicate much. The
existence of streams not much either.
We *could* however consider a different wait time with an empty ring
buffer and one with data. When you know your application, and it runs
at 2k req/s, you know that it's largely sufficient to wait for new
streams to come for 1-20ms depending on bursts. When it gets much less
traffic and the servers are slow, you might want to wait longer. But
when there's data in the ring, you know you'd rather try to get them
out so it makes sense to wait longer. And that would implicitly cover
the reconnect attempts and the "buffer flushed" notification, which
here would just shorten the grace period. We could then have this in
a ring section (or possibly a TCP log line):
grace 1000 empty 100
And the way to prevent this thing from stopping would then "just" be
to increment "jobs" when the ring is started, and decrementing it when
it stops.
> 4. What I believe the right shape is
> ====================================
>
> Take the forwarder out of the exit condition instead of teaching it to
> guess. If a live forwarder is accounted in unstoppable_jobs, then
> (jobs - unstoppable_jobs) == 0 becomes usable as the close signal.
> Strictly speaking it means "everything stoppable is done", which is a
> superset of the data plane, but that is the conservative side to err on,
> and it is computed by the core with no heuristic in sink.c, no
> proxy-type special case and no polling. It does widen what
> unstoppable_jobs counts beyond the master CLI listener; if you would
> rather keep that counter's meaning, a separate counter playing the same
> role works just as well.
Given what I mentioned above about peers, I think it's OK to increment
jobs when the sink is started and decrement it later when the grace
period expires after stopping is signaled.
> Note this also settles the question you raised further down: at that
> point no stream is left to produce new messages, so "the ring is empty"
> is a final state rather than a race, and there is nothing left to sample
> or confirm.
No, it's really more difficult: the ring is empty because log servers
consume the contents fast, but an existing stream might still be
finishing. Maybe that *is* protected via actconn, though. I'm not
yet completely convinced, because there's a TOCTOU here: you see
actconn==0, but immediately after you looked, a frontend does a final
accept() and gets a connection that was pending in the accept queue.
Thus I'd rather say that we're "reasonably confident" that no more
logs will arrive after a few ms once seeing an empty buffer and
actconn==0, but that is not a guarantee and that's why a short
delay is needed here.
(...)
> One detail though: an explicit "ring"
> section gets no server timeout at all by default. sink_setup_proxy()
> leaves timeout.server at TICK_ETERNITY, and only the implicit rings
> created from a "log" directive get the 1s/5s defaults in
> sink_new_from_logger(). So for the stopping-time bound to be reliable it
> needs a sane default of its own rather than inheriting "no limit".
Indeed!
> > And if it had been established then closed ?
>
> Unchanged from today: the stream aborts, the endpoint gets
> SE_FL_EOS|SE_FL_ERROR and the session is torn down as usual, and since
> process_sink_forward() only creates sessions while !stopping, no
> reconnection happens.
This really is my concern. In a mode where we'd close frontends and
sinks and you forward locally between the same process, you'd see
the front close the connection to the ring, that doesn't reconnect,
and its logs would be lost. Thus I think we need to keep the ability
to reconnect during the grace period: the sink continues to work
perfectly normally during this period.
> The ring contents
> cannot reach the new process either -- memory rings die with the worker,
> and file-backed rings are rotated at startup rather than resumed.
Interesting point. I *think* that the ring format could support multiple
writers if we didn't rotate it. It's MPMC in design, and since it was
initially designed for traces, the goal was precisely to rotate it so as
not to lose traces. But I think it sounds perfectly possible for the new
process to scan it like haring does, find the end, and plug there,
continuing to produce at the end. However, there will be no indicator of
where to restart reading, so possibly some old logs would be resent. So
that requires deeper thinking I guess.
> shut(WR) and waiting for the client rather than resetting sounds right
> to me, and a stopping-specific bound on top of it even more so. I would
> rather leave that one to you since it is about frontend soft-stop
> semantics in general rather than about sinks. My log-forward exclusion
> then goes away on its own.
OK.
> 7. Proposed split
> =================
>
> 1. sink: on soft-stop, flush what is already in the ring before
> closing, without waiting for new messages. Fixes (a) only. The
> worker lives just long enough to write out the existing backlog,
> bounded by (2); no new exit condition.
> 2. sink: bound that stopping-time flush with a stopping-specific
> timeout rather than a hardcoded delay.
So see my points above, I think there is still some food on the table to
make sure everything is properly addressed.
> 3. core + sink: keep the forwarder alive until the stoppable data
> plane is done, per section 4. Fixes (b). Needs your call on the
> hook point.
> 4. (yours) frontend option to shut down connections on soft-stop.
>
> I can send 1 and 2 whenever you want. I would rather not write 3 before
> we agree on its shape.
I want that we're certain of the design before discussion patches,
because what gets merged is hard to change later once users adopt it.
And the current no-reconnect approach is a no-go to me, just like the
absence of server-side timeout, that might be OK when everything is
running but no longer on soft-stop for example.
Willy
