On Fri, Jul 31, 2026 at 08:07:28PM +0900, ygkat wrote: > Hi Willy, > > Thanks, this settles the shape for me: a time-based grace, no state > detection, and actconn out of the picture entirely. It also removes the > question I had about a flush hook on the exit path. If the sink holds a > job, the process simply has not reached its exit condition yet and the > flush happens in the normal poll loop, so nothing special is needed > there. > > Let me write out what I understood so you can correct it before I write > any code.
It's really painful that instead of participating to the discussion point by point you always repost a new wall of text. Each time it takes me 20mn to read your prose and I feel like I'm reading the output of an LLM. I'll try to get back to this early next week now, I still have other reviews waiting. thanks, Willy > > 1. Semantics > ============ > > Per forwarding sink, two durations, both only meaningful once stopping > is set: > > grace <ms> how long the sink may keep the worker alive > empty <ms> how long it waits for a new message while the ring is > empty > > and during that window the sink behaves exactly as it does outside of > soft-stop today: it dispatches, it waits on the ring, and it reconnects. > > when the sink enters its grace: > deadline_grace = now + grace > deadline_empty = unset > > on each wakeup during the grace: > if the ring is not empty, or something was forwarded since the > last wakeup: > deadline_empty = unset > else if deadline_empty is unset: > deadline_empty = now + empty > > if deadline_empty or deadline_grace has expired: > close the forwarders, release the job > > So "the ring stayed empty for <empty> ms" is the normal way out, and > <grace> is the hard cap that covers a downstream which is slow or gone. > A message arriving at any point puts us back in the first case. > > I wrote <empty> as a sliding window rather than a single deadline > computed once at stopping, because a one-shot deadline would close the > sink right after a message that arrived and was drained just before it > fired, which contradicts the paragraph above. Tell me if you meant it > one-shot. > > Two definitions to pin down, since both are ambiguous as soon as you > look at the code: > > - with several servers on the ring, each target keeps its own read > position, so "the ring is empty" has to be read per target: every > target has forwarded everything written to the ring. A target still > holding backlog, for instance one that is reconnecting, keeps the > empty window unarmed and we run to the grace cap. I believe that is > what you meant by wanting to wait longer when there is data. > > - "when the sink enters its grace" is when its forward_task gets the > stopping broadcast, not when the stopping flag is set. With a global > "grace" configured those are different instants: soft_stop() sets > stopping immediately and do_soft_stop_now() only runs after the > global delay. Keying the sink off its own armed state rather than > off the global flag keeps the two from interacting. > > One consequence worth stating plainly, since I over-claimed in the > opposite direction last time: a sliding <empty> closes on the first > quiet gap longer than <empty>. A straggler whose log lands after such a > gap, a long poll on a low-traffic instance for example, is still lost no > matter how large <grace> is. That is the trade-off you described, and I > think it is the right one; I just do not want to pretend the design > guarantees more than it does. > > 2. Where the job is taken > ========================= > > Agreed on incrementing jobs, and on taking it when the sink starts > rather than when the stopping signal arrives. > > I had drafted the peers variant first, taking it on the signal like the > dont_stop latch in __process_stopping_peer_sync(), before noticing that > it reintroduces the same family of race you mentioned for peers: > do_soft_stop_now() releases the listeners' jobs in protocol_stop_now() > before broadcasting signal 0, so on an otherwise idle process jobs can > reach zero and the exit condition can be met while the sink's > forward_task is still queued and has not taken its job yet. Taking it at > startup has no such window. > > A sink with no grace configured takes no job at all, so its behaviour > stays exactly today's. The only visible effect of holding one from > startup is the "Jobs" counter of "show info" being one higher per > forwarding sink, since jobs is otherwise only compared in the exit > condition. > > The deadlines and the periodic re-check still belong on the sink's > forward_task, which is already woken by the stopping broadcast as it is > registered on signal 0. That is also the fix for the busy-loop I > reported: the re-check has to drive forward_task->expire, which > process_sink_forward() already manages, and never appctx->t->expire. > > One job per sink rather than one per forwarding target, released when > one of the two deadlines fires. > > One interaction worth mentioning: hard_stop() shuts all streams down, > ours included, sets killed and re-arms itself one second later; its > second pass logs "Some tasks resisted to hard-stop", sets killed to 2, > and the poll loop exits on that. So we never hang. But in between, with > the grace still armed, the forward_task would cheerfully reconnect the > forwarders hard_stop has just killed, which is both noisy and > self-defeating. Ending the grace as soon as killed is set avoids it. > > 3. Reconnecting during grace > ============================ > > Agreed that no-reconnect is wrong. There is a detail in the way though: > process_sink_forward() allows at most one connection attempt per second > per target (sft->last_conn + 1s, last_conn being set when the session is > created). With "grace 1000", a target that loses its connection during > the window gets its next attempt up to a second after the previous one, > so at the very end of the window at best, and past it, therefore never, > at worst. The guarantee you are asking for would not actually hold at > small grace values. > > Skipping the tempo for the first attempt after a disconnection during > the grace looks like the smallest change, and it keeps the one-per- > second pace for repeated failures. I would do that unless you prefer > clamping the tempo instead. > > 4. Naming and placement > ======================= > > "grace" is still taken at the global level, and it means the other end > of the same operation: global "grace <time>" delays the soft-stop itself > after SIGUSR1 (global.grace_delay). A ring-level "grace" would mean > "keep going for this long once the soft-stop has started". Having both > in one configuration file seems likely to confuse. > > It is your call and I do not mind either way; I just did not want us to > discover it after the fact. If you would rather avoid it, something in > the timeout family reads naturally next to the "timeout connect" and > "timeout server" a ring section already accepts. > > You also mentioned the setting could live on a TCP log line. I would > start with the ring section only, give the implicit sinks created from > "log" lines the same default as everyone else, and add a per-log-line > override later if it turns out to be wanted. Those implicit sinks are > the common case, so if you would rather have the override from the > start, say so and I will do it in the same patch. > > 5. Defaults > =========== > > One thing left to decide: whether grace defaults to zero. > > Zero keeps today's behaviour for everyone and makes the whole thing > opt-in. A non-zero default fixes the common case at a fairly modest > cost: streams hold jobs anyway, so the worker already lives until its > last stream finishes, which means the sink typically only adds about > <empty> ms after the last message, and the full <grace> is only reached > when the downstream is stuck or slow. > > I would still start at zero so that a backport changes nothing, and > revisit the default separately once the mechanism is in. Tell me if you > would rather have it non-zero from the start. > > Separately, and independent of all this: an explicit "ring" section gets > no default "timeout connect" or "timeout server" at all, both stay at > TICK_ETERNITY unless set explicitly, while sink_new_from_logger() gives > the implicit rings 1s/5s. Giving the explicit ones the same defaults > looks like a small standalone fix, useful outside soft-stop too. > > 6. On the point I got wrong > =========================== > > You are right about the TOCTOU and I withdraw the claim. actconn == 0 > does not mean no more logs, since a connection sitting in the accept > queue can be accepted right after the read. As the design no longer > looks at actconn at all, the log-forward exclusion, the drain sampling > and the ring-lock ordering argument all disappear with it. Good > riddance. > > 7. Where that leaves the series > =============================== > > 1. MINOR: sink: give an explicit "ring" section the default connect > and server timeouts. Independent of the rest. > 2. MEDIUM: sink: the grace mechanism above. Takes a job when the sink > starts, keeps dispatching and reconnecting during the grace, > releases on whichever deadline fires first. Fixes both (a) and (b). > 3. (yours) the frontend option. > > That is one patch for the sink side rather than the three I proposed > last time. Splitting "flush the backlog" from "wait for the stragglers" > would only have produced an intermediate behaviour nobody wants, and > your design covers both with one mechanism anyway. > > On resuming a file-backed ring in the new process: I agree it looks > possible, and I agree about the missing read position. I would rather > leave it out of this series. > > So, to summarise what I need from you: confirmation that section 1 is > what you meant, and your preference on the naming. The rest I have > proposed an answer for and will follow unless you say otherwise. > > Thanks, > ygkat
