fix: only allow one activeWindow process to spawn at a time

[anormananderson]

(Hopefully) addresses HASEL-UZH/PersonalAnalytics#430

The PR replaces a setInterval-based polling loop with a recursive setTimeout pattern. This is the correct architectural fix: setInterval fires regardless of whether the previous async call has finished, which can cause multiple concurrent activeWindow calls. The new pattern chains the next timeout only after the current one completes.

Some considerations:

• whether the additional race condition should be addressed (in theory I think it can happen, but I am not sure how often, and also not sure whether it is worth the additional complexity of something like a generation counter. Maybe there is a better way?)
• if not using a generation counter, the comment about it may not be needed
• perhaps loop() deserves a better name?

Explanation for why setInterval() was problematic in its original use case

[casaout]

@anormananderson Thank you very much for your PR - we really hope that it fixes the battery/high-CPU load issue :)

I've made three commits to implement your suggested changes (Rename loop, add generation counter and setting a timeout to prevent the tracker being blocked when the getwindow-call takes longer than 5 seconds.

@anormananderson @grigor-dochev Could you please briefly review the changes to see whether they make sense?

Afterwards, I could create a private release that we could test for a few days (or you could check it out and build locally). Thank you!

[royru]

Hi! Great to see some progress on the battery issues :)

I quickly skimmed through the code, and while there is lots of good stuff overall, I am not 100% convinced it will fully address the issue many are experiencing (I hope I am wrong :D)

• As I understand, you are trying to fix main (=get-windows) processes from piling up. However, on older macOS versions, this process is short-lived (1s maybe), so there's no pile up anyways. Ideally, we should fix the problem that causes main to live seemingly forever.
• I might be (hopefully) wrong, but a new main processe (spawned by calling activeWindow() on L57) is not terminated by the Promise.race() being rejected after the timeout. Again, maybe my understanding is off, but this would mean that we are not really solving the problem unfortunately.

[casaout]

I quickly skimmed through the code, and while there is lots of good stuff overall, I am not 100% convinced it will fully address the issue many are experiencing (I hope I am wrong :D)

• As I understand, you are trying to fix main (=get-windows) processes from piling up. However, on older macOS versions, this process is short-lived (1s maybe), so there's no pile up anyways. Ideally, we should fix the problem that causes main to live seemingly forever.
• I might be (hopefully) wrong, but a new main processe (spawned by calling activeWindow() on L57) is not terminated by the Promise.race() being rejected after the timeout. Again, maybe my understanding is off, but this would mean that we are not really solving the problem unfortunately.

hey @royru Thanks for your review and thoughts! I was also wondering about that. It does seem to help overcome issues that Norman identified. So I suggest that for a few days, we'll first test this fix and discuss further steps (if necessary) in the coming days.

[anormananderson]

Like @royru mentioned, I think commit 0706d82 will cause multiple main processes spawned from activeWindow() to run concurrently (in cases where the main process hangs up, the second setTimeout promise will be resolved and execution will move on, but it won't actually kill the main process).

I didn't realize there are multiple issues with main calls --- my issue was (is?) different than @royru described --- activeWindow()'s main is not living indefinitely, it is just running so slowly that additional calls are piling up concurrently. The original change I made tried to avoid this issue by calling activeWindow() (which spawns the next main process) only after the previous call had succeeded or failed. But this has the drawback that captures will stop if activeWindow() never returns, which sounds like it won't solve this second "indefinite main" case...

I am intrigued because in my testing activeWindow() always returned. It seemed like the screen capture service was eventually timing out on its own ... so maybe further testing is warranted to see how both issues are affected by limiting to one main call at a time?

If the screen capture process doesn't always time out on its own, then I think the best solution would be to kill the underlying main process directly. Unfortunately, get-windows doesn't seem to provide a way to kill it. So I think doing so would require modifying get-windows' internals. For example, maybe since child_process.execFile() [used in get-windows/lib/macos.js] can accept an AbortSignal as a parameter, activeWindow() could be extended with this pattern (or something similar) to make the function time out?

Hope that makes sense. This issue's behaviour is very strange.

[anormananderson]

I did some brief testing and it seems that the Promise.race added by commit 0706d82 is causing mains to pile up for me in some situations. Without that commit the suggested changes work properly in my local environment. At this point I think it is ok to wait for activeWindow to time out on its own; in my testing the function seems to block replayd anyway (until the stuck main returns, if I make more calls to activeWindow they all fail without returning window data as well).

If waiting for activeWindow to time out by itself doesn't resolve the indefinite main issue as well, then the only comprehensive solution I am seeing is to kill the underlying main process(es) spawned by activeWindow. Maybe someone who can reproduce can confirm that issue still exists on commit 44578fc (i.e. without the Promise.race change)?