Re: [PATCH] fsmonitor: flush pending FSEvents before cookie wait

[Patrick Steinhardt <ps@pks.im>]

On Tue, Aug 11, 2026 at 11:22:01AM -0400, Tamir Duberstein wrote:
> On Wed, Aug 5, 2026 at 3:59 AM Patrick Steinhardt <ps@pks.im> wrote:
> > On Tue, Jul 21, 2026 at 05:04:56PM -0400, Tamir Duberstein wrote:
> > > 56cef9cb1a (fsmonitor: use pthread_cond_timedwait for cookie wait,
> > > 2026-04-15) limits the cookie wait to one second so that a filesystem
> > > which never delivers events cannot hang fsmonitor clients. A client that
> > > times out receives a trivial response and scans the entire index.
> > >
> > > FSEvents can defer delivery while it batches notifications and does not
> > > guarantee that its queue is drained in one latency interval. A loaded
> > > macOS system can therefore time out even though the event stream is
> > > working.
> > >
> > > On an Apple M4 Max (16 cores, 128 GiB RAM) running macOS 26.5.2, two
> > > worktrees with a 1,001,178-entry index timed out 484 of 545 and 297 of
> > > 365 fsmonitor requests. One status call performed 934,519 lstat() calls
> > > during a 47-second preload and took 52 seconds overall.
> > >
> > > Ask FSEvents to flush pending notifications after creating the cookie
> > > and before starting the timed wait. Use the asynchronous form because
> > > the client handler holds main_lock, which the listener callback also
> > > acquires. Keep the timeout and the behavior of the other backends
> > > unchanged.
> >
> > I cannot really say much about the FSEvent interfaces, but to me it
> > feels quite reasonable to flush the queue when we are waiting for events
> > to be delivered. And that's exactly what `FSEventStreamFlushAsync()`
> > does: it basically overrides the latency we have configured (which is
> > 1ms) and asks the kernel to flush stuff immediately.
> >
> > > diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> > > index 4161dd8282..8e32b5ae5e 100644
> > > --- a/builtin/fsmonitor--daemon.c
> > > +++ b/builtin/fsmonitor--daemon.c
> > > @@ -206,6 +206,9 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(
> > >       close(fd);
> > >       unlink(cookie_pathname.buf);
> > >
> > > +     /* The listener callback takes main_lock, so this must not block. */
> > > +     fsm_listen__flush_async(state);
> > > +
> > >       /*
> > >        * Wait for the listener thread to observe the cookie file.
> > >        * Time out after a short interval so that the client
> >
> > Okay, so we've unlinked the cookie file and the next thing is that we're
> > waiting for all events to have been processed. As said, it feels
> > reasonable that we're flushing all events before we start waiting for
> > them.
> >
> > What I find surprising though is that this is supposed to make a
> > difference at all. The latency we pass to `FSEventStreamCreate()` is
> > 1 millisecond, and we wait up to 1 second for the cookie event. I would
> > have expected that batching events for 1 milliseconds should be totally
> > fine when we're waiting for a full second anyway.
> >
> > So given that I cannot verify this at all and that I have no clue about
> > the FSEvent interfaces... do you have any explanation why the flush
> > seems to help regardless?
> >
> > I _think_ you're already hinting at this in the commit message, where
> > you say that it's not guaranteed that the queue is drained in a single
> > latency interval. Is there any documentation that tells us what the
> > provided guarantees are?
> >
> > Other than that the code changes look sensible to me, thanks!
> >
> > Patrick
> 
> The following was generated by my coding agent and fact checked and
> edited by me mainly to address you in the second person.
> 
[snip]
> 
> Hope that's helpful.

Sorry, but that's not quite helpful. The questions I'm asking are to
verify whether you understand the consequences and subtleties around the
code area that you're proposing to change. If I wanted to only learn
about this myself then I could simply ask an agent myself, but that's
not really the intent of a code review.

So what I'm looking for is _your_ explanation, not the explanation of
AI. Your explanation may of course be informed by AI. But if so it's
your responsibility to double-check its assumptions, build your own
model and then share your informed opinion with us.

Right now I don't yet have the feeling that you understand why this
fixes the underlying issue.

Thanks!

Patrick