Merge pull request #236 from Pouzor/fix/scan-stop-cancellation

fix: make scan stop interrupt in-flight nmap range (#218)

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}
 
 

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

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

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 []