From 5a17311eb8281f62bf6fa7a46e0b5d35fca8e5f0 Mon Sep 17 00:00:00 2001 From: wind Date: Sat, 28 Feb 2026 19:59:42 +0100 Subject: [PATCH] feat: concurrent DNS resolution and partial scan on nmap failure (#50 #51) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #51 — Concurrent hostname resolution: - Replace sequential SIGALRM loop with ThreadPoolExecutor (max 20 workers) - All PTR lookups run in parallel — eliminates serial ~1-2s DNS overhead - socket.setdefaulttimeout(2) per thread replaces signal-based timeout - signal/contextmanager imports removed (not safe in threads) - Test: 5-host batch all resolved correctly via concurrent mock #50 — Partial scan persists ARP results when nmap fails: - ScanResult gains warnings: list[str] field - orchestrate_scan() catches RuntimeError from run_nmap_scan(), appends warning, continues with ARP-only results (no longer raises) - scan_runner sets scan.warning_message when warnings present - Scan model gains warning_message: Text nullable column - ScanOut schema and _scan_to_out updated to include warning_message - Frontend Scan type updated; Dashboard last-scan card shows ⚠ warning - HistoryPage scan table shows ⚠ icon (tooltip) on rows with warnings - Test: mock nmap failure → ARP device persisted, scan completed + warning Closes #50 Closes #51 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- backend/app/api/__init__.py | 2 ++ backend/app/models/scan.py | 2 ++ backend/app/scan_runner.py | 2 ++ backend/app/scanner/__init__.py | 16 +++++++-- backend/app/scanner/dns_lookup.py | 51 ++++++++++++---------------- backend/tests/test_enrichment.py | 14 ++++++++ backend/tests/test_scan_runner.py | 32 +++++++++++++++++ frontend/src/pages/DashboardPage.tsx | 5 +++ frontend/src/pages/HistoryPage.tsx | 12 ++++++- frontend/src/types/api.ts | 1 + 10 files changed, 103 insertions(+), 34 deletions(-) diff --git a/backend/app/api/__init__.py b/backend/app/api/__init__.py index 13752ff..f6e87c9 100644 --- a/backend/app/api/__init__.py +++ b/backend/app/api/__init__.py @@ -49,6 +49,7 @@ class ScanOut(BaseModel): duration_seconds: float | None devices_found: int | None error_message: str | None + warning_message: str | None risks_critical: int | None risks_high: int | None risks_medium: int | None @@ -142,6 +143,7 @@ def _scan_to_out(s) -> ScanOut: # noqa: ANN001 — SQLAlchemy instance duration_seconds=s.duration_seconds, devices_found=s.devices_found, error_message=s.error_message, + warning_message=s.warning_message, risks_critical=s.risks_critical, risks_high=s.risks_high, risks_medium=s.risks_medium, diff --git a/backend/app/models/scan.py b/backend/app/models/scan.py index 118e1a7..bdd25ca 100644 --- a/backend/app/models/scan.py +++ b/backend/app/models/scan.py @@ -20,6 +20,8 @@ class Scan(Base): duration_seconds = Column(Float, nullable=True) devices_found = Column(Integer, nullable=True) error_message = Column(Text, nullable=True) + warning_message = Column(Text, nullable=True) + # "completed" scans may have a warning_message when nmap failed but ARP succeeded # Risk counts snapshotted at scan completion risks_critical = Column(Integer, nullable=True) risks_high = Column(Integer, nullable=True) diff --git a/backend/app/scan_runner.py b/backend/app/scan_runner.py index 121cdd0..b13721b 100644 --- a/backend/app/scan_runner.py +++ b/backend/app/scan_runner.py @@ -80,6 +80,8 @@ def run_scan_and_persist(triggered_by: str = "scheduler") -> int: scan.risks_high = risk_counts["high"] scan.risks_medium = risk_counts["medium"] scan.risks_low = risk_counts["low"] + if result.warnings: + scan.warning_message = "; ".join(result.warnings) db.commit() logger.info("Scan %d completed: %d devices", scan_id, devices_found) diff --git a/backend/app/scanner/__init__.py b/backend/app/scanner/__init__.py index 3f67818..c3d07ee 100644 --- a/backend/app/scanner/__init__.py +++ b/backend/app/scanner/__init__.py @@ -29,6 +29,9 @@ class ScanResult: arp_only: list[ArpHost] = field(default_factory=list) """Hosts found by arp-scan that nmap returned no data for.""" + warnings: list[str] = field(default_factory=list) + """Non-fatal warnings accumulated during the scan (e.g. nmap failure).""" + def orchestrate_scan( interface: str | None = None, @@ -56,8 +59,15 @@ def orchestrate_scan( arp_by_ip = {h.ip: h for h in arp_hosts} logger.info("Starting nmap scan on %d hosts", len(ip_list)) - nmap_hosts = run_nmap_scan(hosts=ip_list, interface=iface) - logger.info("nmap scan returned %d hosts", len(nmap_hosts)) + nmap_hosts: list[NmapHost] = [] + warnings: list[str] = [] + try: + nmap_hosts = run_nmap_scan(hosts=ip_list, interface=iface) + logger.info("nmap scan returned %d hosts", len(nmap_hosts)) + except RuntimeError as exc: + msg = f"nmap unavailable — showing ARP-only results: {exc}" + logger.warning(msg) + warnings.append(msg) # Enrich nmap results with MAC / vendor from arp-scan nmap_ips: set[str] = set() @@ -75,4 +85,4 @@ def orchestrate_scan( # Hosts arp found but nmap returned nothing for (e.g. ICMP-filtered) arp_only = [h for h in arp_hosts if h.ip not in nmap_ips] - return ScanResult(hosts=nmap_hosts, arp_only=arp_only) + return ScanResult(hosts=nmap_hosts, arp_only=arp_only, warnings=warnings) diff --git a/backend/app/scanner/dns_lookup.py b/backend/app/scanner/dns_lookup.py index 624023c..5875abd 100644 --- a/backend/app/scanner/dns_lookup.py +++ b/backend/app/scanner/dns_lookup.py @@ -9,64 +9,55 @@ from __future__ import annotations import logging -import signal import socket -from contextlib import contextmanager +from concurrent.futures import ThreadPoolExecutor from app.scanner.nmap_scan import NmapHost logger = logging.getLogger(__name__) -_LOOKUP_TIMEOUT_SECONDS = 1 - - -@contextmanager -def _timeout(seconds: int): - """SIGALRM-based timeout context for blocking socket calls.""" - - def _handler(signum, frame): # noqa: ANN001 — signal handler signature is fixed by the stdlib - raise TimeoutError - - old = signal.signal(signal.SIGALRM, _handler) - signal.alarm(seconds) - try: - yield - finally: - signal.alarm(0) - signal.signal(signal.SIGALRM, old) +_LOOKUP_TIMEOUT_SECONDS = 2 +_MAX_WORKERS = 20 def _rdns(ip: str) -> str: """ Return the PTR hostname for *ip*, or empty string on any failure. - Uses a SIGALRM timeout so a single slow DNS server cannot stall - the entire scan cycle. + Uses socket.setdefaulttimeout so each lookup is individually bounded. """ try: - with _timeout(_LOOKUP_TIMEOUT_SECONDS): - hostname, _, _ = socket.gethostbyaddr(ip) - return hostname - except (OSError, TimeoutError): + socket.setdefaulttimeout(_LOOKUP_TIMEOUT_SECONDS) + hostname, _, _ = socket.gethostbyaddr(ip) + return hostname + except OSError: return "" + finally: + socket.setdefaulttimeout(None) def resolve_hostnames(hosts: list[NmapHost]) -> None: """ Fill in missing hostnames on *hosts* via reverse DNS (in-place). - Only queries IPs where nmap returned no hostname. Each lookup is - individually timeout-guarded so a non-responsive resolver doesn't - block the scan. + Only queries IPs where nmap returned no hostname. All lookups run + concurrently via a thread pool so a slow resolver doesn't serialise + the scan cycle. """ missing = [h for h in hosts if not h.hostname] if not missing: return logger.debug("Reverse DNS lookup for %d host(s) with no hostname", len(missing)) + + futures = {} + with ThreadPoolExecutor(max_workers=min(_MAX_WORKERS, len(missing))) as pool: + for host in missing: + futures[pool.submit(_rdns, host.ip)] = host + resolved = 0 - for host in missing: - name = _rdns(host.ip) + for future, host in futures.items(): + name = future.result() if name: host.hostname = name resolved += 1 diff --git a/backend/tests/test_enrichment.py b/backend/tests/test_enrichment.py index 65d41e6..ca5f084 100644 --- a/backend/tests/test_enrichment.py +++ b/backend/tests/test_enrichment.py @@ -211,3 +211,17 @@ def test_empty_list_is_noop(self): with patch("app.scanner.dns_lookup.socket.gethostbyaddr") as mock_dns: resolve_hostnames([]) mock_dns.assert_not_called() + + @pytest.mark.unit + def test_resolves_multiple_hosts_concurrently(self): + """All hosts in the list should be resolved regardless of concurrency.""" + hosts = [NmapHost(ip=f"192.168.1.{i}", hostname="") for i in range(1, 6)] + + def fake_rdns(ip): + return (f"host-{ip.split('.')[-1]}.local", [], [ip]) + + with patch("app.scanner.dns_lookup.socket.gethostbyaddr", side_effect=fake_rdns): + resolve_hostnames(hosts) + + for i, host in enumerate(hosts, 1): + assert host.hostname == f"host-{i}.local" diff --git a/backend/tests/test_scan_runner.py b/backend/tests/test_scan_runner.py index b91dbf3..dfeca4d 100644 --- a/backend/tests/test_scan_runner.py +++ b/backend/tests/test_scan_runner.py @@ -233,3 +233,35 @@ def test_run_scan_populates_risk_counts(in_memory_session_factory, monkeypatch): assert scan.risks_medium >= 0 # type: ignore[union-attr] assert scan.risks_low >= 0 # type: ignore[union-attr] db.close() + + +@pytest.mark.unit +def test_partial_scan_persists_arp_devices_when_nmap_fails(in_memory_session_factory, monkeypatch): + """When nmap fails, ARP-discovered devices must still be persisted and + the scan marked 'completed' with a warning_message.""" + from app.models.device import Device + from app.models.scan import Scan + from app.scan_runner import run_scan_and_persist + from app.scanner import ScanResult + from app.scanner.arp_scan import ArpHost + + monkeypatch.setattr("app.scan_runner.SessionLocal", in_memory_session_factory) + + partial_result = ScanResult( + hosts=[], + arp_only=[ArpHost(ip="192.168.1.50", mac="aa:bb:cc:dd:ee:ff", vendor="Acme")], + warnings=["nmap unavailable — showing ARP-only results: nmap failed"], + ) + + with patch("app.scan_runner.orchestrate_scan", return_value=partial_result): + scan_id = run_scan_and_persist("manual") + + db = in_memory_session_factory() + scan = db.get(Scan, scan_id) + assert scan.status == "completed" # type: ignore[union-attr] + assert scan.warning_message is not None # type: ignore[union-attr] + assert "nmap" in scan.warning_message.lower() # type: ignore[union-attr] + + devices = db.query(Device).filter(Device.ip_address == "192.168.1.50").all() + assert len(devices) == 1 + db.close() diff --git a/frontend/src/pages/DashboardPage.tsx b/frontend/src/pages/DashboardPage.tsx index 9acafcb..f3cf438 100644 --- a/frontend/src/pages/DashboardPage.tsx +++ b/frontend/src/pages/DashboardPage.tsx @@ -302,6 +302,11 @@ export function DashboardPage() { {lastScan.devices_found !== 1 ? "s" : ""} found )} + {lastScan.warning_message && ( + + ⚠ {lastScan.warning_message} + + )} ) : (

diff --git a/frontend/src/pages/HistoryPage.tsx b/frontend/src/pages/HistoryPage.tsx index 48a0a31..735ce0b 100644 --- a/frontend/src/pages/HistoryPage.tsx +++ b/frontend/src/pages/HistoryPage.tsx @@ -284,7 +284,17 @@ export function HistoryPage() { {shortDate(s.started_at)} - {s.triggered_by} + + {s.triggered_by} + {s.warning_message && ( + + ⚠ + + )} + {s.duration_seconds != null ? `${s.duration_seconds}s` diff --git a/frontend/src/types/api.ts b/frontend/src/types/api.ts index 42a183b..ba0eeda 100644 --- a/frontend/src/types/api.ts +++ b/frontend/src/types/api.ts @@ -32,6 +32,7 @@ export interface Scan { duration_seconds: number | null; devices_found: number | null; error_message: string | null; + warning_message: string | null; risks_critical: number | null; risks_high: number | null; risks_medium: number | null;