mirror of
https://github.com/usestrix/strix.git
synced 2026-11-01 07:02:32 +00:00
fix(dep-verify): run the advisory check off the event loop (Greptile P1)
verify_dependency does blocking HTTP (requests.post) and was called directly from the async create_dependency_report path — a slow/unreachable provider would block the shared agent event loop for the request timeout, stalling every concurrent agent (worst case two sequential calls on the clean-version path). - Offload the call via asyncio.to_thread so the event loop stays free. - Tighten the provider timeout 20s -> 10s (named OsvProvider._TIMEOUT_S): a single advisory lookup shouldn't hold a worker thread that long, and it fails open fast. - Add an async test asserting the verifier runs on a worker thread (not the main/ event-loop thread) and the reject still propagates. Addresses the Greptile review comment on #963. 38 report/dep tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This commit is contained in:
parent
d963bbd9ef
commit
43a2886c6e
3 changed files with 54 additions and 3 deletions
|
|
@ -91,7 +91,13 @@ class OsvProvider:
|
|||
"""AdvisoryProvider backed by an OSV-schema /v1/query endpoint. Default
|
||||
api.osv.dev; point `url` at a self-hosted OSV mirror (identical contract) for
|
||||
air-gapped / data-residency deployments. Uses `requests` (a core dep) — no
|
||||
extra package."""
|
||||
extra package.
|
||||
|
||||
The caller runs this off the event loop (asyncio.to_thread), but keep the
|
||||
per-request timeout short so a stalled provider fails open quickly rather than
|
||||
holding a worker thread — this is a bounded advisory lookup, not bulk I/O."""
|
||||
|
||||
_TIMEOUT_S = 10
|
||||
|
||||
def __init__(self, url: str) -> None:
|
||||
self.url = url
|
||||
|
|
@ -99,7 +105,7 @@ class OsvProvider:
|
|||
def _query(self, payload: dict[str, Any]) -> list[dict] | None:
|
||||
try:
|
||||
ca = os.environ.get("REQUESTS_CA_BUNDLE")
|
||||
resp = requests.post(self.url, timeout=20, json=payload,
|
||||
resp = requests.post(self.url, timeout=self._TIMEOUT_S, json=payload,
|
||||
verify=ca if ca else True)
|
||||
if resp.status_code != 200:
|
||||
logger.info("dep-verify: OSV %s for %s; can't answer",
|
||||
|
|
|
|||
|
|
@ -830,7 +830,11 @@ async def _do_create_dependency( # noqa: PLR0912
|
|||
if load_settings().dep_verify.enabled:
|
||||
from strix.report.dep_verify import verify_dependency
|
||||
|
||||
dep_verdict = verify_dependency({
|
||||
# verify_dependency does blocking HTTP (advisory provider). Run it in a
|
||||
# worker thread so a slow/unreachable provider can't stall the shared
|
||||
# async agent loop (it would otherwise block every concurrent agent for
|
||||
# up to the provider timeout, x2 on the clean-version path).
|
||||
dep_verdict = await asyncio.to_thread(verify_dependency, {
|
||||
"cve": parsed_cve, "package_name": package_name,
|
||||
"installed_version": installed_version,
|
||||
"package_ecosystem": package_ecosystem,
|
||||
|
|
|
|||
|
|
@ -2,10 +2,14 @@
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import threading
|
||||
import time
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
import pytest
|
||||
|
||||
import strix.report.dep_verify as depv
|
||||
from strix.config import loader
|
||||
from strix.report.dedupe import (
|
||||
_check_dependency_duplicate,
|
||||
_prepare_report_for_comparison,
|
||||
|
|
@ -192,6 +196,43 @@ async def test_dependency_report_with_zero_cvss_remains_low_severity(
|
|||
assert report["cvss"] == 0.0
|
||||
|
||||
|
||||
async def test_dependency_verify_runs_off_the_event_loop(
|
||||
report_state: ReportState,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""The verifier does blocking HTTP; it must be offloaded (asyncio.to_thread)
|
||||
so a slow/unreachable provider can't stall the shared async agent loop. Stub
|
||||
verify_dependency with a blocking call that records its thread + rejects, and
|
||||
assert (a) the reject propagates and (b) it ran on a NON-main thread."""
|
||||
monkeypatch.setenv("STRIX_DEP_VERIFY", "1")
|
||||
loader._cached = None
|
||||
|
||||
main_thread = threading.current_thread()
|
||||
ran_on: dict[str, object] = {}
|
||||
|
||||
def _blocking_verify(_candidate: dict) -> dict:
|
||||
ran_on["thread"] = threading.current_thread()
|
||||
time.sleep(0.05) # simulate a slow provider (blocking)
|
||||
return {"success": False, "verify_rejected": True,
|
||||
"dep_version_out_of_range": True, "error": "out of range"}
|
||||
|
||||
monkeypatch.setattr(depv, "verify_dependency", _blocking_verify)
|
||||
|
||||
result = await _do_create_dependency(
|
||||
title="CVE-2021-44906 in minimist 1.2.6",
|
||||
description="Prototype pollution.", target="repo/package.json",
|
||||
cve="CVE-2021-44906", package_name="minimist", installed_version="1.2.6",
|
||||
impact="x", remediation_steps="upgrade", assumptions="a",
|
||||
package_ecosystem="npm", fixed_version=None, cwe=None,
|
||||
advisory_cvss=7.5, technical_analysis=None, fix_effort="low",
|
||||
)
|
||||
# reject propagated + finding NOT persisted
|
||||
assert result["verify_rejected"] is True
|
||||
assert not report_state.vulnerability_reports
|
||||
# ran off the event loop (a worker thread, not the main thread)
|
||||
assert ran_on["thread"] is not main_thread
|
||||
|
||||
|
||||
async def test_dependency_report_requires_advisory_cvss(report_state: ReportState) -> None:
|
||||
result = await _do_create_dependency(
|
||||
title="CVE-2024-0001 in sample 1.0.0",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue