Ensure safe_sleep tries alternative approaches by TingluoHuang · Pull Request #4146 · actions/runner · GitHub
Skip to content

Ensure safe_sleep tries alternative approaches#4146

Merged
TingluoHuang merged 1 commit intomainfrom
users/tihuang/fixsleep
Dec 11, 2025
Merged

Ensure safe_sleep tries alternative approaches#4146
TingluoHuang merged 1 commit intomainfrom
users/tihuang/fixsleep

Conversation

@TingluoHuang
Copy link
Copy Markdown
Member

@TingluoHuang TingluoHuang requested a review from a team as a code owner December 11, 2025 04:33
Copilot AI review requested due to automatic review settings December 11, 2025 04:33
@TingluoHuang TingluoHuang changed the title Ensure safe_sleep tries alternative approaches before falling back to… Ensure safe_sleep tries alternative approaches Dec 11, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the safe_sleep.sh script by implementing a cascading fallback strategy for sleep functionality. Instead of immediately falling back to a CPU-intensive busy-wait loop, the script now attempts multiple alternative approaches in order of preference.

Key Changes:

  • Adds primary check for the standard sleep command before attempting alternatives
  • Implements a ping-based sleep as a secondary fallback
  • Adds a read -t timeout-based sleep for bash environments as a tertiary option
  • Retains the existing busy-wait loop as the final fallback
Comments suppressed due to low confidence (2)

src/Misc/layoutroot/safe_sleep.sh:5

  • The parameter $1 is not validated before use. If the script is called without arguments or with invalid input (non-numeric, negative numbers, or special characters), it could cause unexpected behavior or errors in the various sleep implementations. Consider adding input validation at the beginning of the script to ensure $1 is a valid positive number.
    sleep "$1"

src/Misc/layoutroot/safe_sleep.sh:37

  • The busy-wait fallback continuously checks the SECONDS variable in a tight loop, which will consume 100% CPU for the duration of the sleep. This is inefficient and can impact system performance, especially for longer sleep durations. While this is a fallback for when no other option is available, consider adding a comment to warn about this behavior, or implement a less aggressive polling mechanism if possible.
SECONDS=0
while [[ $SECONDS -lt $1 ]]; do
    :
done

Comment thread src/Misc/layoutroot/safe_sleep.sh
Comment thread src/Misc/layoutroot/safe_sleep.sh
Comment thread src/Misc/layoutroot/safe_sleep.sh
Comment thread src/Misc/layoutroot/safe_sleep.sh
Comment thread src/Misc/layoutroot/safe_sleep.sh
Comment thread src/Misc/layoutroot/safe_sleep.sh
Comment thread src/Misc/layoutroot/safe_sleep.sh
Comment thread src/Misc/layoutroot/safe_sleep.sh
@multitrack-collector
Copy link
Copy Markdown

multitrack-collector commented Dec 11, 2025

@TingluoHuang TingluoHuang merged commit b2204f1 into main Dec 11, 2025
18 checks passed
@TingluoHuang TingluoHuang deleted the users/tihuang/fixsleep branch December 11, 2025 14:53
@viktorlott
Copy link
Copy Markdown

This isn't TTY-agnostic, because the read -t fallback only works when stdin/stdout/stderr are TTYs. You can break it pretty easy executing the script without TTYs:
PATH= ./safe_sleep.sh 5 </dev/null >/dev/null 2>/dev/null.

Also, read is a buildin keyword, so no need to check if it exists.

Here's a more robust solution:

#!/usr/bin/env bash

secs=${1:-0}

if command -v sleep >/dev/null 2>&1; then
    sleep "$secs"
    exit 0
fi

if [ -n "${BASH_VERSION-}" ]; then
    # Bash >= 4 has coproc, which gives us a TTY-agnostic approach using pure bash builtins.
    if [ "${BASH_VERSINFO[0]}" -ge 4 ] 2>/dev/null; then
        coproc _slp { read; }
        read -t "$secs" <&"${_slp[0]}" || :
        kill "$_slp_PID" 2>/dev/null || :
        wait "$_slp_PID" 2>/dev/null || :
        exit 0
    fi

    # Creates a PTY with tail instead.
    if command -v tail >/dev/null 2>&1; then
        exec 3< <(tail -f /dev/null)
        read -t "$secs" <&3 || :
        exec 3<&-
        exit 0
    fi

    # If we have a controlling TTY.
    if exec 3</dev/tty 2>/dev/null; then
        read -t "$secs" <&3 || :
        exec 3<&-
        exit 0
    fi
fi

if command -v ping >/dev/null 2>&1; then
    ping -c "$((secs + 1))" 127.0.0.1 >/dev/null 2>&1 || :
    exit 0
fi

SECONDS=0
while (( SECONDS < secs )); do
    :
done

There are actually a bunch of tail alternatives that can be used instead that also creates PTYs.

@multitrack-collector
Copy link
Copy Markdown

This isn't TTY-agnostic, because the read -t fallback only works when stdin/stdout/stderr are TTYs. You can break it pretty easy executing the script without TTYs: PATH= ./safe_sleep.sh 5 </dev/null >/dev/null 2>/dev/null.

Also, read is a buildin keyword, so no need to check if it exists.

Here's a more robust solution:

#!/usr/bin/env bash

secs=${1:-0}

if command -v sleep >/dev/null 2>&1; then
    sleep "$secs"
    exit 0
fi

if [ -n "${BASH_VERSION-}" ]; then
    # Bash >= 4 has coproc, which gives us a TTY-agnostic approach using pure bash builtins.
    if [ "${BASH_VERSINFO[0]}" -ge 4 ] 2>/dev/null; then
        coproc _slp { read; }
        read -t "$secs" <&"${_slp[0]}" || :
        kill "$_slp_PID" 2>/dev/null || :
        wait "$_slp_PID" 2>/dev/null || :
        exit 0
    fi

    # Creates a PTY with tail instead.
    if command -v tail >/dev/null 2>&1; then
        exec 3< <(tail -f /dev/null)
        read -t "$secs" <&3 || :
        exec 3<&-
        exit 0
    fi

    # If we have a controlling TTY.
    if exec 3</dev/tty 2>/dev/null; then
        read -t "$secs" <&3 || :
        exec 3<&-
        exit 0
    fi
fi

if command -v ping >/dev/null 2>&1; then
    ping -c "$((secs + 1))" 127.0.0.1 >/dev/null 2>&1 || :
    exit 0
fi

SECONDS=0
while (( SECONDS < secs )); do
    :
done

There are actually a bunch of tail alternatives that can be used instead that also creates PTYs.

I mean you could submit another PR

@multitrack-collector
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants