From da2c1c356a04472de19360cff15b251f4cc34b69 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Sun, 28 Jun 2026 11:13:14 +0200 Subject: [PATCH] fix: make scan stop interrupt in-flight nmap range Stop button had no effect: run_scan only checked the cancel flag between CIDR ranges and between hosts, never inside the per-range nmap call. For a single /24 the whole scan is one blocking call, so cancel was ignored for minutes and the run status stayed 'running'. - thread run_id into _nmap_scan/_ping_sweep/_nmap_port_scan; check cancel before each phase and skip queued hosts once cancelled - flip ScanRun status to 'cancelled' eagerly in the stop endpoint so the UI reacts immediately instead of waiting for a checkpoint Fixes #218 ha-relevant: yes --- backend/app/api/routes/scan.py | 8 +++- backend/app/services/scanner.py | 32 ++++++++++++--- backend/tests/test_scan.py | 6 ++- backend/tests/test_scanner.py | 70 ++++++++++++++++++++++++++++++++- 4 files changed, 107 insertions(+), 9 deletions(-) diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index 0ef6398..1b3fd06 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -1,7 +1,7 @@ import ipaddress import logging import uuid -from datetime import datetime +from datetime import datetime, timezone from typing import Any from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException @@ -191,6 +191,12 @@ async def stop_scan( if run.status != "running": raise HTTPException(status_code=409, detail="Scan is not running") request_cancel(run_id) + # Flip status eagerly so the UI reflects the stop immediately, instead of + # waiting for run_scan to reach its next cancellation checkpoint (which may + # be blocked inside a long nmap call). run_scan converges to the same state. + run.status = "cancelled" + run.finished_at = datetime.now(timezone.utc) + await db.commit() return {"stopping": True} diff --git a/backend/app/services/scanner.py b/backend/app/services/scanner.py index 07401e8..2287773 100644 --- a/backend/app/services/scanner.py +++ b/backend/app/services/scanner.py @@ -175,7 +175,7 @@ def _arp_table_hosts(network: str) -> dict[str, dict[str, Any]]: return {} -async def _ping_sweep(target: str) -> dict[str, dict[str, Any]]: +async def _ping_sweep(target: str, run_id: str | None = None) -> dict[str, dict[str, Any]]: """ Phase 1: Concurrent ICMP ping sweep + ARP cache. Pings all IPs in the CIDR in parallel (up to 50 at once, 1s timeout each). @@ -205,6 +205,12 @@ async def _ping_sweep(target: str) -> dict[str, dict[str, Any]]: alive_ips: set[str] = {ip for ip in ping_results if ip is not None} logger.info("[Phase 1] %d/%d hosts responded to ping", len(alive_ips), len(all_ips)) + # Cancelled during the sweep — bail before the (potentially long) Phase 2 + # port scan. Returning empty makes _nmap_scan skip nmap entirely. + if run_id is not None and _is_cancelled(run_id): + logger.info("[Phase 1] %s — scan cancelled, skipping hostname/ARP enrichment", target) + return {} + # ARP cache: catch devices that block ICMP but were recently active, # and enrich ping-alive hosts with their MAC addresses. arp_cache = await asyncio.to_thread(_arp_table_hosts, target) @@ -286,7 +292,8 @@ def _nmap_scan_single(host_dict: dict[str, Any], port_spec: str = _EXTRA_PORTS) async def _nmap_port_scan( - alive: dict[str, dict[str, Any]], port_spec: str = _EXTRA_PORTS + alive: dict[str, dict[str, Any]], port_spec: str = _EXTRA_PORTS, + run_id: str | None = None, ) -> list[dict[str, Any]]: """ Phase 2: Per-IP service detection with bounded concurrency. @@ -301,6 +308,11 @@ async def _nmap_port_scan( async def _scan_with_sem(host_dict: dict[str, Any]) -> dict[str, Any]: async with semaphore: + # Once cancelled, skip the expensive nmap call for every host still + # queued behind the semaphore — return it unscanned so the gather + # unwinds fast instead of blocking the stop for minutes. + if run_id is not None and _is_cancelled(run_id): + return host_dict return await asyncio.to_thread(_nmap_scan_single, host_dict, port_spec) raw = await asyncio.gather(*[_scan_with_sem(h) for h in alive.values()], return_exceptions=True) @@ -314,24 +326,32 @@ async def _nmap_port_scan( return results -async def _nmap_scan(target: str, port_spec: str = _EXTRA_PORTS) -> list[dict[str, Any]]: +async def _nmap_scan( + target: str, port_spec: str = _EXTRA_PORTS, run_id: str | None = None +) -> list[dict[str, Any]]: """ Two-phase scan for a CIDR range. Phase 1: Concurrent ping sweep to find alive hosts (fast, no false positives). Phase 2: Per-IP nmap port scan with service detection (bounded concurrency, 10 at a time). + + ``run_id`` lets each phase poll for cancellation so a stop request takes + effect mid-range instead of only at CIDR/host boundaries in run_scan. """ logger.info("[Scan] Starting scan for %s — nmap available: %s", target, _NMAP_AVAILABLE) + if run_id is not None and _is_cancelled(run_id): + logger.info("[Scan] %s — cancelled before start, skipping", target) + return [] if not _NMAP_AVAILABLE: logger.warning("[Scan] nmap not available — returning mock data") return _mock_scan(target) try: - alive = await _ping_sweep(target) + alive = await _ping_sweep(target, run_id=run_id) logger.info("[Phase 1] Found %d alive host(s) in %s: %s", len(alive), target, ", ".join(sorted(alive.keys()))) except Exception as exc: logger.error("Phase 1 ping sweep failed: %s", exc) raise RuntimeError(str(exc)) from exc - return await _nmap_port_scan(alive, port_spec) + return await _nmap_port_scan(alive, port_spec, run_id=run_id) async def _mdns_discover(timeout: float = 4.0) -> list[dict[str, Any]]: @@ -563,7 +583,7 @@ async def run_scan( for cidr in ranges: if _is_cancelled(run_id): break - hosts = await _nmap_scan(cidr, port_spec) + hosts = await _nmap_scan(cidr, port_spec, run_id=run_id) for host in hosts: if _is_cancelled(run_id): break diff --git a/backend/tests/test_scan.py b/backend/tests/test_scan.py index 3b05eb0..2114bb1 100644 --- a/backend/tests/test_scan.py +++ b/backend/tests/test_scan.py @@ -652,6 +652,10 @@ async def test_stop_scan_success(client: AsyncClient, headers, db_session: Async assert res.json() == {"stopping": True} # run_id added to cancel set assert run.id in _cancelled_runs + # status flipped eagerly so the UI reacts without waiting for a checkpoint + await db_session.refresh(run) + assert run.status == "cancelled" + assert run.finished_at is not None # cleanup for other tests _cancelled_runs.discard(run.id) @@ -691,7 +695,7 @@ async def test_run_scan_cancelled_mid_scan_skips_remaining_cidrs(db_session: Asy call_count = 0 - def nmap_side_effect(target: str, port_spec: str | None = None): + def nmap_side_effect(target: str, port_spec: str | None = None, run_id: str | None = None): nonlocal call_count call_count += 1 # Signal cancellation after the first CIDR scan completes diff --git a/backend/tests/test_scanner.py b/backend/tests/test_scanner.py index c493a7d..5153282 100644 --- a/backend/tests/test_scanner.py +++ b/backend/tests/test_scanner.py @@ -277,6 +277,74 @@ async def test_nmap_scan_raises_on_sweep_error(): await _nmap_scan("192.168.1.0/24") +# --------------------------------------------------------------------------- +# Cancellation responsiveness (issue #218) +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_nmap_scan_cancelled_before_start_skips_phases(): + """A run already cancelled returns immediately without touching the network.""" + from app.services.scanner import _cancelled_runs, _nmap_scan, request_cancel + + run_id = "cancel-before-start" + request_cancel(run_id) + try: + with patch("app.services.scanner._ping_sweep", new_callable=AsyncMock) as mock_sweep, \ + patch("app.services.scanner._nmap_port_scan", new_callable=AsyncMock) as mock_port: + result = await _nmap_scan("192.168.1.0/24", run_id=run_id) + assert result == [] + mock_sweep.assert_not_called() + mock_port.assert_not_called() + finally: + _cancelled_runs.discard(run_id) + + +@pytest.mark.asyncio +async def test_ping_sweep_cancelled_mid_sweep_returns_empty(): + """Cancelling during Phase 1 bails before Phase 2 — no alive hosts returned.""" + from app.services.scanner import _cancelled_runs, _ping_sweep, request_cancel + + run_id = "cancel-during-sweep" + + async def _fake_subprocess(*args, **kwargs): + proc = AsyncMock() + proc.wait = AsyncMock(return_value=1) + proc.returncode = 1 + return proc + + request_cancel(run_id) + try: + with patch("app.services.scanner.asyncio.create_subprocess_exec", new=_fake_subprocess), \ + patch("app.services.scanner._arp_table_hosts", return_value={}): + result = await _ping_sweep("192.168.1.0/30", run_id=run_id) + assert result == {} + finally: + _cancelled_runs.discard(run_id) + + +@pytest.mark.asyncio +async def test_nmap_port_scan_skips_queued_hosts_when_cancelled(): + """Once cancelled, queued hosts return unscanned instead of invoking nmap.""" + from app.services.scanner import _cancelled_runs, _nmap_port_scan, request_cancel + + run_id = "cancel-port-scan" + alive = { + "192.168.1.10": { + "ip": "192.168.1.10", "mac": None, "hostname": None, + "os": None, "open_ports": [], + }, + } + request_cancel(run_id) + try: + with patch("app.services.scanner._nmap_scan_single") as mock_single: + result = await _nmap_port_scan(alive, run_id=run_id) + mock_single.assert_not_called() + assert result[0]["ip"] == "192.168.1.10" + assert result[0]["open_ports"] == [] + finally: + _cancelled_runs.discard(run_id) + + # --------------------------------------------------------------------------- # _mdns_discover # --------------------------------------------------------------------------- @@ -665,7 +733,7 @@ async def test_run_scan_deep_scan_passes_port_spec_to_nmap(mem_db): captured = {} - async def fake_nmap(target, port_spec): + async def fake_nmap(target, port_spec, run_id=None): captured["port_spec"] = port_spec return []