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

On Tue, Aug 11, 2026 at 12:45 PM Tamir Duberstein <[email protected]> wrote:
>
> On Tue, Aug 11, 2026 at 12:23 PM Patrick Steinhardt <[email protected]> wrote:
> >
> > On Tue, Aug 11, 2026 at 11:22:01AM -0400, Tamir Duberstein wrote:
> > > On Wed, Aug 5, 2026 at 3:59 AM Patrick Steinhardt <[email protected]> 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.
>
> Got it. I agree with you that the flush call feels unnecessary under
> the interpretation that passing 1ms to FSEventStreamCreate is the
> equivalent of asking it to flush every 1ms. Empirically, though,
> that's not the case, as described in the commit message.
>
> There's more precedent for this technique (found by agent, sorry):
> watchman fixed a similar issue here:
> https://github.com/facebook/watchman/commit/d1795de4ecab33672a89802318fe6f0122462194
> and the documented it here:
> https://github.com/facebook/watchman/commit/2f80886991ce81585ac0679c2b019fa0e4d9e9dd
>
> I agree this is unsatisfying.
>
> Does that help?

I did a bunch more digging and I'm withdrawing this patch. I haven't
succeeded in proving that this fixes the performance issue. I'll
resend in case this changes.

Thanks all for pushing back!
Tamir