macOS - sleep inhibiting

[kiminuo]

Contributes (hopefully) to #9339

Based on:

• I disrupted all the rounds last night #9339 (comment)
• I disrupted all the rounds last night #9339 (comment)

Not tested, I can test it in the afternoon

[molnard]

I am not sure what is the actual fix idea here. It seems like a huge refactor.

[kiminuo]

To run caffeinate -i for the duration when it is necessary (when CJ is in progres). Exactly as we do that on linux.

It's certainly not a huge refactor. There is just a new common base class for linux & mac task.

[molnard]

To run caffeinate -i for the duration when it is necessary (when CJ is in progres). Exactly as we do that on linux.

I am not sure that timing is causing the problem here. If I caffeinated the system every 5 seconds and asked to stay awake for a minute then the end result should be to stay awake. Anyway, your test will show if this makes sense. Can you reproduce the original issue on your system?

It's certainly not a huge refactor. There is just a new common base class for linux & mac task.

It seems to be. 7 files modified. As a reviewer, it takes time to see what is the actual change.

[kiminuo]

I am not sure that timing is causing the problem here. If I caffeinated the system every 5 seconds and asked to stay awake for a minute then the end result should be to stay awake.

I tried this approach because Clement mentioned it here #9339 (comment).

Can you reproduce the original issue on your system?

I can try again to reproduce it in the afternoon. So far I had issues with reproducing it though.

It's certainly not a huge refactor. There is just a new common base class for linux & mac task.

It seems to be. 7 files modified. As a reviewer, it takes time to see what is the actual change.

True but that's why there are commits explaining it step by step.

[molnard]

I am testing this.

[turbolay]

This PR solves the issue I had while testing #9339
It still disrupts a round if I'm in critical state and closes the lid, but I guess this is inevitable apart with low level functions like sudo pmset disablesleep 1.

[molnard]

This PR solves the issue I had while testing #9339

Awesome, one step ahead - let's review this.

[turbolay]

So this PR doesn't solve the issue even with the last commit because of this:

https://github.com/zkSNACKs/WalletWasabi/blob/0ee438d7a1fe86f69ce4e0a3a9760b85275f3b19/WalletWasabi/Services/SleepInhibitor.cs#L98
which should be something like

if (TaskFactory is not null)

With this, the PR solves the issue and kills properly the caffeinate process when needed.

Notes:

• https://github.com/zkSNACKs/WalletWasabi/blob/0ee438d7a1fe86f69ce4e0a3a9760b85275f3b19/WalletWasabi/Helpers/EnvironmentHelpers.cs#L254-L259
  This does not work well and anyway with your PR it will never be used, it should be removed

• Prolong() is called repeatedly. Maybe it's useful on other plateforms, but not on mac. Prolong has no effect FWIU

• Killing the process with ctrl + c shows the logs:

2022-11-27 17:08:52.217 [49] ERROR	BaseInhibitorTask.WaitAsync (56)	Inhibit process ended prematurely.
2022-11-27 17:08:52.217 [49] ERROR	BaseInhibitorTask.WaitAsync (79)	Process had already existed.

[molnard]

Keep it up, guys - this is important!

[kiminuo]

@turbolay You are right. Thanks for pointing it out.

I have verified that with the latest commit, I can see caffeinate -i in the list of processes when I start a CJ process and if I pause it, then the caffeinate -i process disappears. So this behaves as expected. I have not tried the scenario of "killing wallet" because my coins would get banned (we need to test this too).

This does not work well and anyway with your PR it will never be used, it should be removed

Yes, removed. Thanks.

• Prolong() is called repeatedly. Maybe it's useful on other plateforms, but not on mac. Prolong has no effect FWIU

Now it should have an effect.

Killing the process with ctrl + c shows the logs:

We are certainly interested in this scenario because we don't want to: kill WW and leave caffeinate -i running. That would be messy for users. This is not solved now, I'm not sure if it by terminating WW, the child processes are terminated too or not.

[molnard]

Is this ready to review? (it is still a draft)

[kiminuo]

Yes

[turbolay]

tACK

Just a note: When I force kill WW, caffeinate is not killed.
We should fix this in a subsequent PR.

[kiminuo]

I've been researching this and unfortunately on macOS there is no tail --pid (https://apple.stackexchange.com/questions/410513/in-mac-how-to-do-tail-f-pid-to-terminate-after-process-exit-like-linux?newreg=93fc09f4822e4a209ca4ac5655e4150c).

One advanced idea how to tackle this is to start a bash script like this on macOS machines:

caffeinate -i &                           # Start caffeinate as a background process.
processPID=$!                           # Get the PID of the caffeinate process.
wait {Environment.ProcessId}    # Wait until Wasabi Wallet is stopped (can this work?)
kill $processPID                         # Kill the caffeinate process. We get here only if somebody kills his WW app instance forcefully.

but then we have a different problem: We can't kill the caffeinate process from .NET because we would kill just the script and not the backgrounded. :) That in turn might be solvable using Process.Kill(entireProcessTree: true)

Another idea was to just start

caffeinate -i wait {Environment.ProcessId}

but that does not work for me. I'm not entirely sure why it does not work.

Resources

• https://ss64.com/osx/wait.html

[turbolay]

# Set up a trap to run the "killall caffeinate" command when the parent process is killed
trap "killall caffeinate" SIGINT SIGTERM

https://ss64.com/osx/trap.html

@kiminuo WDYT?

EDIT: So I tested it, it does not work and I think that's because it's a shell built in command and we have UseShellExecute = false,. /usr/bin/trap does not exist and therefore when executing the process you have Win32Exception: File not found. Maybe there is some kind of workarounds but I don't know.

I tried:

FileName = "/bin/bash",
Arguments = "-c 'trap \"killall caffeinate\" SIGINT SIGTERM'",

In that case the killall command is executed almost immediately, I think it does not work because of context.

[molnard]

pmset works similarly?

[turbolay]

pmset works similarly?

yes
It works perfectly except in case of SIGKILL (force quit) Wasabi. In that case the process is not killed, exactly as caffeinate.

2022-12-06_06-50-34.mp4

[turbolay]

@kiminuo this script is similar to the one you posted but should work as intended (just change process name):

#!/usr/bin/env bash

# Trap the killall so we kill the background caffeinate when Process.Kill() is executed in C#
trap "killall caffeinate" SIGINT SIGTERM

caffeinate -i &
CAFFEINATE_PID=$!

# Loop until wasabi is killed without sending SIGINT SIGTERM
while [ -n "$(pgrep Wasabi)" ]
do
    sleep 1
done

# Kill caffeinate in case that wasabi was forced killed 
kill $CAFFEINATE_PID && exit

If it doesn't work directly we should just add the SIGINT flag in the Process.Kill()

[kiminuo]

Hm, wouldn't this work?

#!/usr/bin/env bash

caffeinate -i &
processPid=$!
trap "kill -9 ${processPid}" SIGINT SIGTERM
wait ${processPid}

It seems conceptually better. It's not tested but I'm interested if you like the concept.

btw: while [ -n "$(pgrep Wasabi)" ] this can kill a different instance, if we ever allow that. We should be strict here imho.

[turbolay]

Hm, wouldn't this work?

#!/usr/bin/env bash

caffeinate -i &
processPid=$!

# Trap to kill the 
trap "kill -9 ${processPid}" SIGINT SIGTERM

wait ${processPid}

It seems conceptually better. It's not tested but I'm interested if you like the concept.

btw: while [ -n "$(pgrep Wasabi)" ] this can kill a different instance, if we ever allow that. We should be strict here imho.

Seems like same idea but better code.
But I think this causes issues when the parent process is forced kill

[turbolay]

I used both of our suggestions to make this code, which I tested and it works in all cases:

Pass the SIGTERM bool in Process.Kil()

Add the bool in the wrapper:
https://github.com/zkSNACKs/WalletWasabi/blob/a275f0c645ea020418a9a8b4b63ff06f4c16dcdc/WalletWasabi/Microservices/ProcessAsync.cs#L71-L74

public virtual void Kill(bool gracefully = false)
{
    Process.Kill(gracefully);
}

Then call with the bool set as True:
https://github.com/zkSNACKs/WalletWasabi/blob/ccd9a280a964bb531b413afed650df03d3c79856/WalletWasabi/Helpers/PowerSaving/BaseInhibitorTask.cs#L68

Process.Kill(true);

Launch the process

var wasabiProcessId = Environment.ProcessId;
string scriptPath = "/path/to/script.sh";
startInfo.FileName = "bash";
startInfo.Arguments = $"{scriptPath} {wasabiProcessId}";

Pass as argument the PID of Wasabi's process

Bash script

#!/usr/bin/env bash
caffeinate -i &
caffeinatePid=$!
trap "kill -9 $caffeinatePid" 0 SIGINT SIGTERM
while [ -n "$(grep $1)" ]
do
    sleep 1
done

The script launches caffeinate in the background, waits for parent process (wasabi) to end. Wasabi now kills with SIGTERM so trap will work if Wasabi want to pause caffeinate. If wasabi is forced kill (SIGKILL received), the bash script will leave the loop and exit with code 0, trap will also catch that and kill caffeinate.
wait was not working for me so I kept the loop

I didn't manage to run the script hardcoded (not relying on path), so I leave that to you.

Just a final note: Using the PID like this is considered unreliable because for whatever reason it might change in long running systems. If you think that this problem is concerning in a way, we should either use the process name or when calling Prolong() check if wasabi's PID changed. Anyways it would be in a subsequent PR because we should merge asap, it would be awesome to have this fix in the Release.

[molnard]

Nice! @kiminuo can you make this happen? caffeinate works fine with the small script.

[kiminuo]

Yeah, I will try.

@turbolay Thanks for this

Just a final note: Using the PID like this is considered unreliable because for whatever reason it might change in long running systems. If you think that this problem is concerning in a way, we should either use the process name or when calling Prolong() check if wasabi's PID changed. Anyways it would be in a subsequent PR because we should merge asap, it would be awesome to have this fix in the Release.

I don't think this is true. Do you have a source of this information? I mean process ID does not change, it's an identifier of the process, you don't have anything else to recognize the process. The only important thing is that after some time the process ID can be re-used and that would be indeed bad if we killed a different process.

[turbolay]

I don't think this is true. Do you have a source of this information? I mean process ID does not change, it's an identifier of the process, you don't have anything else to recognize the process. The only important thing is that after some time the process ID can be re-used and that would be indeed bad if we killed a different process.

You are right, it cannot change but can be reused.
We could kill a different process or not kill caffeinate (if a process with same pid than wasabi is running).
I don't think it's too concerning anyway

[molnard]

@kiminuo I asked @turbolay to create a quick addon PR to this that implements the script. Do not work on this PR - to avoid double work.

[turbolay]

@molnard I coordinated with kimi, he already did it!