Introduce an alternative Linux implementation using inotify…

[savetheclocktower]

…that fits our feature set better than EFSW’s does and doesn’t have the complexity of recursive watching.

In pulsar-edit/pulsar#1592 we're dealing with a crash whose origin seems to be a memory allocation issue somewhere within pathwatcher — specifically EFSW’s inotify implementation. We could chase it down further, but we'd basically be debugging vendor code at that point, and I'd much rather dedicate the same level of effort toward a replacement.

A while back, when EFSW’s macOS implementation didn't fit our needs, I wrote an API-compatible FSEvents adapter that we could use in its place. (Then, much more recently, when FSEvents seemed to mean regressions in certain cases, I wrote a separate kqueue-based adapter to use instead.)

This PR extends this idea further by adding a new inotify-based adapter for Linux that can be used instead of EFSW's inotify implementation. Our adapter has the advantage of being much simpler because pathwatcher doesn't do recursive filesystem watching; it cares only about directories and their direct children.

I cannot promise that this itself will prevent the crashes reported in pulsar-edit/pulsar#1592, but it should at the very least move them toward code that we maintain and can understand much better.

The tests pass on my Linux VM; if they pass in CI, I'll make a draft PR in the Pulsar repo that points to this pathwatcher branch simply for the purpose of generating binaries that affected users can download to see whether this fixes the issue.

[savetheclocktower]

Tentatively good news — one user affected by pulsar-edit/pulsar#1592 says that this fixes it for them. So after this lands, I'll be motivated to write a Windows-specific adapter as well so that we can finally retire EFSW.

I'm grateful for EFSW’s existence (I tried rewriting pathwatcher back in 2024 and it didn't go well until I brought EFSW into the mix!) but we probably don't need it anymore now that we can write simpler adapters for our narrow use cases. And it's looking like pathwatcher will stay with us (in one form or another) instead of being retired, so we might as well make it light and robust.

[savetheclocktower]

Paging @Daeraxa as an “unofficial” reviewer. (We should get you added to the appropriate list so you can do real reviews!)

[Daeraxa]

Paging @Daeraxa as an “unofficial” reviewer. (We should get you added to the appropriate list so you can do real reviews!)

I will be happy to review as soon as I can, sorry I've been absurdly busy recently and won't be able to look properly until Monday, but I will look!

[Daeraxa]

So the binary produced definitely seems to work, I haven't managed to get it all working directly via yarn start but that's because I clearly haven't updated it in other packages used in Pulsar - specifically tree-view.

[savetheclocktower]

I haven't managed to get it all working directly via yarn start but that's because I clearly haven't updated it in other packages used in Pulsar - specifically tree-view.

If you're building from pulsar-edit/pulsar#1597 that's supposed to be taken care of. As long as you run yarn install && yarn build after checking out, it should update all usages of @pulsar-edit/node-pathwatcher to use the same copy. If it's still giving you trouble after that, let me know.

[Daeraxa]

aah hadnt realised it was in a pr, still working through things... lots to catch up on.

Huh, dunno what went wrong, I did the same as in that PR but it didn't resolve correctly - no idea what the issue was there. Either way it seems to work just fine.

Do you know of any other place where we might have seen this issue other than tree-view?

[Daeraxa]

Humour my ignorance here, if this is an API compatible replacement then why do we still need to include this? Or is it only a particular part being replaced?

[savetheclocktower]

The .hpp file being referenced includes type definitions. So we need it in order to be able to refer to things below like efsw::FileWatchListener and efsw::WatchID.

But, yes, in another sense only a particular part is being replaced. In core.h we conditionally define the FileWatcher type as referring to either an efsw::FileWatcher (on Windows) or an API-compatible replacement (on other platforms). In order to prove it's an API-compatible replacement, we have to reference efsw‘s types.

I imagine that we'll eventually use our own implementation on Windows, at which point we can remove efsw from the repo entirely and just use types entirely of our own definition.

[Daeraxa]

Just to ask this again:

Do you know of any other place where we might have seen this issue other than tree-view?

Just want to test any other areas that might be affected by this change, if they exist. Otherwise I'm happy to rubber stamp it as fixing the main issue it solves.

[savetheclocktower]

I don't. The main places we use pathwatcher are

• In tree-view (each directory gets its own watcher when it is expanded);
• In text-buffer (which uses pathwatcher to find out when buffers were modified by other programs);
• As part of the API we expose to package authors (the File and Directory classes).

I wish I could give you a better theory of the bug than “this change doesn't result in a crash anymore.” Claude had a couple of theories, but I can't find one right now. My best guess is that this was a bug in the C++ code that was exacerbated by something that tree-view was doing — maybe calling back into pathwatcher directly within a change handler that was invoked by pathwatcher. I bet there exists a reduce test case that would not need to involve pathwatcher at all, but I haven't been able to come up with it.

[savetheclocktower]

This one will get landed soon if nobody objects. We're going to bump pathwatcher to at least 6.0.4 in Pulsar 1.133.0 in order to fix atomic-save recognition, so it'd be great to get this fix in as well.

[savetheclocktower]

OK, I did a re-review of this PR with Claude (who had helped with the initial adapter authoring as well) to make sure it's bulletproof. It raised some issues that made sense to fix, including one that was worth writing a new spec to cover:

When someone asks to watch /foo/bar/baz.js, the JS layer tells the adapter to instead watch /foo/bar if we're on Linux or Windows. This was a preexisting pattern that helped us (in #6) to improve handling of the “atomic save” pattern that lots of tools use. We don't care about following the same logical file by its inode (or some other platform-specific ID); we care about following changes to the path that we specified.

But these platforms are susceptible to the same problem, moved up one layer. Even if they watch /foo/bar, they must account for the fact that /foo/bar itself could be renamed! Suppose it were moved to /thud/bar; if we didn't detect and account for this, we'd go on happily reporting changes to events within the folder, even though that folder no longer lives where it did when we started watching it.

This was big enough to be worth fixing on Linux, whether or not the bug had been there even before this PR. So we fixed it and wrote a new spec to cover it. But the new spec failed on Windows, too, so we needed to fix that in this PR as well.

On Linux, the fix is pretty easy; we get notified when the directory is renamed. We can stop the watcher directly when that happens.

On Windows, it's harder, because we do not get notified upon rename. So we'd have to wait until we process an event on that renamed path (like a child being modified) — but when that happens, we sanity-check the watcher's current path to ensure it matches whatever it was when we started the watcher. If not, we ignore the event and stop the watcher.

On macOS, we already had platform-specific code that did the required path sanity-checking; so this spec already passed.

---

In order to get the new spec to pass on Windows, we needed to touch vendored EFSW code… but that's fine. It's already on our roadmap to replace EFSW altogether now that only Windows relies on it, so there will be no ongoing maintenance headache here because this patch only has to survive until that replacement happens.