Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 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
download_git_repository cancelled the watchdog that download_git_clone had just started, so raise_if_timed_out() was unreachable for git downloads: on Windows the Alarm was cancelled before it could fire, on POSIX SIGALRM was disarmed before the clone began. run_git_clone_with_size_guard was left with no wall-clock bound at all - a stalled clone (network hang, credential wait) looped in proc.communicate() forever, and a partially cloned target dir was reported as a successful download. The early cancellation was there to avoid the watchdog's os._exit(1), which no longer happens: the watchdog now only kills the tracked child and lets the caller raise TimeOutException. - remove the early _cancel_download_watchdog() call - re-raise TimeOutException from run_git_clone_with_size_guard instead of letting the broad except degrade it to a generic git error, and clean the partial target dir on the way out - skip the default-branch fallback clone once the watchdog fired; the budget is spent and the watchdog is one-shot, so a retry would run unbounded A stalled clone now fails at SIGNAL_TIMEOUT with 'Timeout (600 sec)' and a cleaned target dir on both platforms, instead of hanging indefinitely.
size_check_after_sec defaulted to SIGNAL_TIMEOUT, so the first mid-clone size check was scheduled for the same instant as the watchdog. The watchdog starts before the clone process, so it always fired first: mid-clone size checks and the periodic SIZE_CHECK_INTERVAL_SECONDS re-checks never ran, and an oversized clone was reported as a timeout instead of a size-limit block. That also lost the caller's size_limit_blocked branch, which then retried the same oversized package over HTTP/wget for another SIGNAL_TIMEOUT. Introduce SIZE_CHECK_AFTER_SECONDS (60) so the size guard aborts oversized clones with its own message well before the wall-clock watchdog fires.
2 participants
Summary by CodeRabbit
Bug Fixes