-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Remove unneeded safe_sleep #4150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Trenton VanderWert <trenton.vanderwert@gmail.com>
Elyytscha
commented
Dec 17, 2025
At the very least, maybe safe_sleep should prefer the system sleep binary when it exists (I think thats how it was designed initially), and only fall back to this bespoke shell-loop masterpiece when it doesn’t.
Much easier to meter runner minutes when your sleep actually wakes up
vtrenton
commented
Dec 17, 2025
At the very least, maybe safe_sleep should prefer the system sleep binary when it exists (I think thats how it was designed initially), and only fall back to this bespoke shell-loop masterpiece when it doesn’t.
Much easier to meter runner minutes when your sleep actually wakes up
PRs have since been merged to reimplement this approach:
I agree it's much better than just the while loop. But my intention of this PR is to remove safe_sleep altogether. As I feel this is overengineering for a problem that doesn't exist. This design has created much more of a problem than it's worth.
If we want to go down the compatibility rabbit hole - we can. Does bash exist? does command exist? should we write a safe_bash and safe_command? when does it stop.
Hey Github Runner team!
I felt the need to open this PR as to at least have a discussion around this. safe_sleep has created serious problems in the past with wasting CPU cycles using #3792 as reference. The codebase keeps getting bigger with hacks like using ping as a timer... Which is great except it relies heavily on the assumption that ping on all systems will never change it send frequency (unlikely - but there is a potential).
There is also an existing concern about the latest fix being vulnerable to injection attacks. Although not likely exploitable - it's new risk introduced into the codebase: #3792 (comment)
There is no need for this as almost zero UNIX-Like systems including containers and Android/Embedded systems don't have sleep. Sleep is a standard POSIX utility on all UNIX systems.
To top it off these scripts require BASH which is installed on less systems than sleep. Let alone hardcoding the shebang path to /bin/bash which systems such as Nix and BSD do not use. BASH is much larger compatibility risk than sleep.
if you're making bash (a much less deployed tool) a hard requirement than sleep should be.
All of this is to say - we should remove safe_sleep from the codebase and just use/assume a POSIX sleep implementation is available. This is technical debt and the implementation costs much more than its worth.
I would like to challenge anyone to give me an example of a UNIX-Like system that uses BASH that doesn't have sleep. Very open to being challenged on this. I seriously for the life of me can think of a single one. Would love to hear any feedback on this!
Thank you for your consideration!