Conversation
📝 WalkthroughWalkthroughThe Windows download watchdog now tracks child processes, records timeout state, and avoids immediate process termination. Git and HTTP/ ChangesDownload timeout handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fosslight_util/download.py`:
- Around line 99-106: Update the timeout parsing helper around the
FOSSLIGHT_DOWNLOAD_TIMEOUT handling to validate parsed non-zero values against
the platform limits supported by signal.alarm() and threading.Event.wait(). When
the value exceeds the supported limit, log it as invalid and return
SIGNAL_TIMEOUT; preserve the existing fallback for missing, blank, or
non-integer values and allow valid zero values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5791e6fb-ceea-43c5-a2ca-a4291a92366c
📒 Files selected for processing (1)
src/fosslight_util/download.py
| raw = os.environ.get("FOSSLIGHT_DOWNLOAD_TIMEOUT") | ||
| if raw is None or raw.strip() == "": | ||
| return SIGNAL_TIMEOUT | ||
| try: | ||
| return int(raw) | ||
| except ValueError: | ||
| logger.warning("Invalid FOSSLIGHT_DOWNLOAD_TIMEOUT=%s; using %d sec.", raw, SIGNAL_TIMEOUT) | ||
| return SIGNAL_TIMEOUT |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import signal
import threading
print("threading.TIMEOUT_MAX =", getattr(threading, "TIMEOUT_MAX", None))
if hasattr(signal, "alarm"):
for value in (2**31, 2**32, 10**20):
try:
signal.alarm(value)
print(f"signal.alarm({value}) accepted")
except (OverflowError, OSError, ValueError) as error:
print(f"signal.alarm({value}) rejected: {error}")
finally:
signal.alarm(0)
PYRepository: fosslight/fosslight_util
Length of output: 438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching download.py:\n'
fd -a 'download\.py$' . | sed 's#^\./##'
printf '\nRelevant section:\n'
sed -n '85,125p' src/fosslight_util/download.py
printf '\nUsages of get_download_timeout:\n'
rg -n "get_download_timeout|FOSSLIGHT_DOWNLOAD_TIMEOUT|signal\.alarm|Thread\(" src/fosslight_util/download.pyRepository: fosslight/fosslight_util
Length of output: 2265
Reject out-of-range download watchdog timeout values.
FOSSLIGHT_DOWNLOAD_TIMEOUT currently accepts values that can make the watchdog unusable. On Python, signal.alarm() rejects values too large to fit in a C int, while threading.Event.wait() still accepts values above threading.TIMEOUT_MAX. If timeout is non-zero but exceeds the supported platform limit, clamp it to SIGNAL_TIMEOUT instead of starting with an invalid value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fosslight_util/download.py` around lines 99 - 106, Update the timeout
parsing helper around the FOSSLIGHT_DOWNLOAD_TIMEOUT handling to validate parsed
non-zero values against the platform limits supported by signal.alarm() and
threading.Event.wait(). When the value exceeds the supported limit, log it as
invalid and return SIGNAL_TIMEOUT; preserve the existing fallback for missing,
blank, or non-integer values and allow valid zero values.
bjk7119
changed the title
✅ Action performedReview finished.
|
soimkim
left a comment
soimkim
left a comment
There was a problem hiding this comment.
머지시 커밋 메세지 본문 부분 필수 수정 필요 건.
그리고 이 수정은 timeout에 대한 이벤트 잡는게 아님.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fosslight_util/download.py (1)
118-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the swallowed exception during process cleanup.
_kill_download_processes()catchesExceptionand discards it silently whenproc.poll()/proc.kill()fails. Log the exception at debug level so a failed cleanup attempt is visible during troubleshooting.♻️ Proposed fix
for proc in procs: try: if proc.poll() is None: proc.kill() - except Exception: - pass + except Exception as error: + logger.debug(f"Failed to kill tracked download process: {error}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fosslight_util/download.py` around lines 118 - 119, Update the exception handler in _kill_download_processes() to log the caught cleanup exception at debug level instead of silently passing, while preserving the existing cleanup flow and exception suppression.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fosslight_util/download.py`:
- Around line 1188-1192: Remove the early _cancel_download_watchdog() call from
download_git_repository so the alarm started by download_git_clone remains
active through run_git_clone_with_size_guard. Preserve the existing
raise_if_timed_out(alarm, target_dir) handling and let the watchdog enforce the
overall clone timeout on both platforms.
---
Nitpick comments:
In `@src/fosslight_util/download.py`:
- Around line 118-119: Update the exception handler in
_kill_download_processes() to log the caught cleanup exception at debug level
instead of silently passing, while preserving the existing cleanup flow and
exception suppression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: faae5b4c-f8e8-47bd-9bcd-94c36dd2d570
📒 Files selected for processing (1)
src/fosslight_util/download.py
| # The watchdog kills the clone on timeout, so the call above returns with a | ||
| # partial checkout. Turn that into TimeOutException instead of reporting a | ||
| # confusing git error, and clean the half-written target directory. | ||
| raise_if_timed_out(alarm, target_dir) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Git-path timeout propagation never fires; the watchdog is already cancelled before the clone starts.
download_git_clone starts the watchdog at Line 1164 (alarm = _start_download_watchdog()), then calls download_git_repository, whose first action (Line 1091-1094, unchanged) is:
try:
_cancel_download_watchdog()
except Exception:
passThis call cancels the same alarm object just started, before run_git_clone_with_size_guard spawns the git subprocess and registers it with register_download_process (Line 995-1002). On Windows, Alarm._cancelled.set() fires before the timeout thread can ever observe a timeout, so alarm.timed_out stays False. On POSIX, signal.alarm(0) disarms SIGALRM before the clone even begins. As a result, raise_if_timed_out(alarm, target_dir) at Line 1191 can never raise TimeOutException for git downloads; it is unreachable dead code in practice.
This also means run_git_clone_with_size_guard has no external time bound. Its own loop only aborts a clone that exceeds size_limit_gb; when the clone stays under the limit (or no limit is set), proc.communicate(timeout=size_check_interval_sec) loops indefinitely. A stalled clone (network hang, credential wait) never times out on either platform.
The size-guard's own default, size_check_after_sec: int = SIGNAL_TIMEOUT (Line 981), suggests the intended design already expects the watchdog and the size-guard to share the same time budget, not for one to disable the other. Given the watchdog's new behavior only kills the tracked child process and raises a graceful TimeOutException (it no longer force-exits the whole interpreter), the original justification for the early cancellation ("Avoid hard process exit from parent watchdog while size-guarded clone may run longer") no longer applies.
Remove the early cancellation so the watchdog stays armed for the whole git clone, matching the wget/HTTP path, which has no equivalent cancellation and where the new timeout handling does work as intended.
🐛 Suggested direction for the fix
logger.info(f"Download git url :{git_url}, version:{refs_to_checkout}")
- # Avoid hard process exit from parent watchdog while size-guarded clone may run longer
- try:
- _cancel_download_watchdog()
- except Exception:
- pass
-
env = os.environ.copy()Do you want me to work out the full fix, including confirming SIGNAL_TIMEOUT is an acceptable overall bound for large clones guarded by size_limit_gb?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fosslight_util/download.py` around lines 1188 - 1192, Remove the early
_cancel_download_watchdog() call from download_git_repository so the alarm
started by download_git_clone remains active through
run_git_clone_with_size_guard. Preserve the existing raise_if_timed_out(alarm,
target_dir) handling and let the watchdog enforce the overall clone timeout on
both platforms.
2 participants
Summary by CodeRabbit
Bug Fixes