From 9daea4c4d04febcff4fe5f5d89d98e3e281c48da Mon Sep 17 00:00:00 2001 From: Pouzor Date: Thu, 20 Aug 2026 23:14:58 +0200 Subject: [PATCH] fix(scan): keep an edit, a discovery source and one name for a failed run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the PR review. A finishing deep rescan threw away an edit in progress. The reset effect in InventoryDeviceModal was keyed on the `device` object, and the parent hands down a fresh one whenever the row is refreshed — including from the poll's own onSaved. Same device, new object, so the effect reset the form and left edit mode, minutes into a scan the user was waiting on. It is keyed on the id now: a refreshed row is not a different device, and only a different device is a reason to throw the form away. The fresh row still reaches the canvas and the grid — gating that behind edit mode would discard the scan result instead, and a save unions the services server-side anyway. A rescan tagged every device it touched as "arp"-discovered, so a Proxmox guest, a rack mount or a hand-added host started answering the network source filter. It carries its own source through instead. The two background wrappers wrote status "failed" where the scanners write "error" for the same condition. Harmonized on "error" — the one Scan History filters and colours; "failed" showed up unlabelled. The frontend union keeps 'failed' for rows already recorded. That last one meant updating an existing assertion in test_scan_run.py: it encoded the old spelling. ha-relevant: yes --- backend/app/api/routes/scan.py | 10 ++++- backend/app/services/scanner.py | 7 ++- backend/tests/scan/test_scan_run.py | 42 +++++++++++++++--- backend/tests/test_scanner.py | 39 +++++++++++++++++ frontend/src/api/client.ts | 2 + .../modals/InventoryDeviceModal.tsx | 25 ++++++++--- .../InventoryDeviceModalRescan.test.tsx | 43 +++++++++++++++++++ 7 files changed, 153 insertions(+), 15 deletions(-) diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index 47955c0..eeb3874 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -183,7 +183,10 @@ async def _background_scan( await db.rollback() run = await db.get(ScanRun, run_id) if run and run.status == "running": - run.status = "failed" + # "error", the same word run_scan / run_device_scan write when + # they fail themselves — one condition, one name. Scan History + # filters and colours that one; "failed" showed up unlabelled. + run.status = "error" await db.commit() @@ -209,7 +212,10 @@ async def _background_device_scan( await db.rollback() run = await db.get(ScanRun, run_id) if run and run.status == "running": - run.status = "failed" + # "error", the same word run_scan / run_device_scan write when + # they fail themselves — one condition, one name. Scan History + # filters and colours that one; "failed" showed up unlabelled. + run.status = "error" await db.commit() diff --git a/backend/app/services/scanner.py b/backend/app/services/scanner.py index 94ad6ef..b437cdc 100644 --- a/backend/app/services/scanner.py +++ b/backend/app/services/scanner.py @@ -856,6 +856,11 @@ async def run_device_scan( "os": device.os, "open_ports": [], } + # A rescan re-observes a device through the source it already has; it + # is not a network discovery. Hardcoding "arp" here would tell a + # Proxmox guest, a rack mount or a hand-added host that the network + # scanner found it, and it would then answer that source filter. + source = device.discovery_source or "arp" # The full range goes out in slices so a stop request lands within one # slice instead of at the end, and so a spent budget keeps the ports @@ -912,7 +917,7 @@ async def run_device_scan( # The row is being rescanned on the user's request, so it is never # "hidden" from itself — the route rejects hidden devices upfront. outcome = await process_host( - db, host, hidden_ips=set(), deep_scan=deep_scan, discovery_source="arp" + db, host, hidden_ips=set(), deep_scan=deep_scan, discovery_source=source ) if outcome == "created": devices_found = 1 diff --git a/backend/tests/scan/test_scan_run.py b/backend/tests/scan/test_scan_run.py index 5fa9108..37793cb 100644 --- a/backend/tests/scan/test_scan_run.py +++ b/backend/tests/scan/test_scan_run.py @@ -12,9 +12,14 @@ from app.services.scanner import _cancelled_runs, request_cancel, run_scan @pytest.mark.asyncio -async def test_background_scan_marks_run_failed_on_exception(mem_db): - """If run_scan() raises, the ScanRun must transition running → failed and the - session rollback path must execute without a follow-on exception.""" +async def test_background_scan_marks_run_errored_on_exception(mem_db): + """If run_scan() raises, the ScanRun must transition running → error and the + session rollback path must execute without a follow-on exception. + + "error" and not "failed": one condition, one name — it is what run_scan + writes when it catches the failure itself, and the only one Scan History + filters and colours. + """ from app.api.routes.scan import _background_scan async with mem_db() as session: @@ -36,13 +41,13 @@ async def test_background_scan_marks_run_failed_on_exception(mem_db): async with mem_db() as session: refreshed = await session.get(ScanRun, run_id) assert refreshed is not None - assert refreshed.status == "failed" + assert refreshed.status == "error" @pytest.mark.asyncio async def test_background_scan_leaves_non_running_status_alone(mem_db): """If the run was already stopped/cancelled before run_scan failed, _background_scan - must NOT overwrite that terminal status with 'failed'.""" + must NOT overwrite that terminal status with 'error'.""" from app.api.routes.scan import _background_scan async with mem_db() as session: @@ -436,3 +441,30 @@ async def test_run_scan_updates_existing_pending_device(db_session: AsyncSession # Services and hostname should be updated assert device.hostname == "myhost.lan" assert any(s["port"] == 8096 for s in device.services) + + +@pytest.mark.asyncio +async def test_background_device_scan_marks_run_errored_on_exception(mem_db): + """Same word as the network scan — a device run that blows up reads alike.""" + from app.api.routes.scan import _background_device_scan + + async with mem_db() as session: + run = ScanRun(status="running", kind="device", ranges=["10.0.0.5/32"]) + session.add(run) + await session.commit() + run_id = run.id + + with ( + patch("app.api.routes.scan.AsyncSessionLocal", mem_db), + patch( + "app.api.routes.scan.run_device_scan", + new_callable=AsyncMock, + side_effect=RuntimeError("boom"), + ), + ): + await _background_device_scan(run_id, "d1") + + async with mem_db() as session: + refreshed = await session.get(ScanRun, run_id) + assert refreshed is not None + assert refreshed.status == "error" diff --git a/backend/tests/test_scanner.py b/backend/tests/test_scanner.py index e0a32c1..22e0ad2 100644 --- a/backend/tests/test_scanner.py +++ b/backend/tests/test_scanner.py @@ -1233,6 +1233,45 @@ async def test_scan_keeps_the_icon_the_user_picked_for_a_service(mem_db): assert ssh["category"] == "remote" +@pytest.mark.asyncio +async def test_run_device_scan_keeps_the_device_own_discovery_source(mem_db): + """A rescan re-observes a device; it does not discover it on the network. + + Tagging every rescanned device "arp" told a Proxmox guest or a hand-added + host that the network scanner found it, and it then answered that filter. + """ + from app.services.scanner import run_device_scan + + run_id = _make_run_id() + async with mem_db() as session: + session.add(ScanRun(id=run_id, status="running", kind="device", ranges=["192.168.1.9/32"])) + session.add(InventoryDevice( + id="d1", + ip="192.168.1.9", + status="approved", + services=[], + discovery_source="proxmox", + discovery_sources=["proxmox"], + )) + await session.commit() + + def _scanned(host_dict, port_spec, bounded=False): + host_dict["open_ports"] = [{"port": 8006, "protocol": "tcp", "banner": ""}] + return host_dict + + async with mem_db() as session: + with patch("app.services.scanner._nmap_scan_single", side_effect=_scanned), \ + patch("app.api.routes.status.broadcast_scan_update", new_callable=AsyncMock): + await run_device_scan("d1", session, run_id, ports="8006") + + async with mem_db() as session: + device = await session.get(InventoryDevice, "d1") + + assert device is not None + assert device.discovery_sources == ["proxmox"] + assert device.discovery_source == "proxmox" + + @pytest.mark.asyncio async def test_run_device_scan_marks_run_error_when_device_has_no_ip(mem_db): from app.services.scanner import run_device_scan diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 0d4b236..392c755 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -112,6 +112,8 @@ export type ScanConfigData = { ranges: string[] } & DeepScanConfig /** A row of `scan_runs` — what `/scan/runs` and the device rescan return. */ export interface ScanRunSummary { id: string + // 'failed' is legacy: runs recorded before the backend settled on 'error' + // for the same condition. Still read, never written. status: 'running' | 'done' | 'cancelled' | 'error' | 'failed' kind: string ranges: string[] diff --git a/frontend/src/components/modals/InventoryDeviceModal.tsx b/frontend/src/components/modals/InventoryDeviceModal.tsx index d316aed..dca522d 100644 --- a/frontend/src/components/modals/InventoryDeviceModal.tsx +++ b/frontend/src/components/modals/InventoryDeviceModal.tsx @@ -279,24 +279,35 @@ export function InventoryDeviceModal({ device, onClose, onApprove, onHide, onIgn onSavedRef.current = onSaved const editingRef = useRef(editing) editingRef.current = editing + // Read by the reset effect, which keys on the id alone — the object itself + // must not be a dependency. + const deviceRef = useRef(device) + deviceRef.current = device - // Reset the form whenever the modal is pointed at another device — the + // Reset the form whenever the modal is pointed at *another* device — the // component stays mounted across card clicks in the inventory grid. + // + // Keyed on the id, never on the object: the parent hands down a fresh + // `device` every time the row is refreshed from anywhere — a finishing deep + // rescan, a status push, a save — and resetting on those would drop an edit + // in progress along with the mode itself. A refreshed row is not a different + // device, and only a different device is a reason to throw the form away. + const deviceId = device?.id ?? null useEffect(() => { - if (!device) return - setForm(toForm(device)) - setProperties(device.properties ?? []) - setServices(device.services ?? []) + const d = deviceRef.current + if (!d) return + setForm(toForm(d)) + setProperties(d.properties ?? []) + setServices(d.services ?? []) setEditing(false) setSvcModal(null) setRescanRunId(null) setDeepScanOpen(false) - }, [device]) + }, [deviceId]) // Wait on a running deep rescan. A full 65535-port scan takes minutes, so the // run is polled rather than awaited — the user can close the modal, and the // scan keeps going and still shows under Scan History. - const deviceId = device?.id ?? null useEffect(() => { if (!rescanRunId || !deviceId) return let stopped = false diff --git a/frontend/src/components/modals/__tests__/InventoryDeviceModalRescan.test.tsx b/frontend/src/components/modals/__tests__/InventoryDeviceModalRescan.test.tsx index 0ad4cdc..2d7bc82 100644 --- a/frontend/src/components/modals/__tests__/InventoryDeviceModalRescan.test.tsx +++ b/frontend/src/components/modals/__tests__/InventoryDeviceModalRescan.test.tsx @@ -160,6 +160,49 @@ describe('InventoryDeviceModal — deep rescan', () => { expect(mockRun.mock.calls.length).toBe(calls) }) + it('keeps an edit in progress when the scan lands', async () => { + // The parent patches the row in place on onSaved (`setSelected(saved)`), + // handing down a new object for the same device. That must not throw away + // a form the user is still filling in — a deep scan runs for minutes. + vi.useFakeTimers({ shouldAdvanceTime: true }) + const onSaved = vi.fn() + const device = makeDevice() + const fresh = makeDevice({ + services: [{ port: 8096, protocol: 'tcp', service_name: 'Jellyfin' }], + }) + const { rerender } = render( + + ) + await startScan() + await waitFor(() => expect(screen.getByTestId('device-rescan-stop')).toBeInTheDocument()) + + fireEvent.click(screen.getByRole('button', { name: 'Edit' })) + fireEvent.change(screen.getByPlaceholderText('Display name'), { target: { value: 'Media box' } }) + + mockRun.mockResolvedValue({ data: { id: 'run-1', status: 'done', error: null } }) + mockPending.mockResolvedValue({ data: [fresh] }) + await act(async () => { await vi.advanceTimersByTimeAsync(3100) }) + + // The fresh row still reaches the canvas and the grid — the scan is not + // discarded just because a form is open. + await waitFor(() => expect(onSaved).toHaveBeenCalledWith(fresh)) + rerender() + + expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument() + expect(screen.getByDisplayValue('Media box')).toBeInTheDocument() + }) + + it('still resets the form when pointed at another device', async () => { + const { rerender } = render() + fireEvent.click(screen.getByRole('button', { name: 'Edit' })) + fireEvent.change(screen.getByPlaceholderText('Display name'), { target: { value: 'Media box' } }) + + rerender() + + expect(screen.queryByRole('button', { name: 'Save' })).toBeNull() + expect(screen.queryByDisplayValue('Media box')).toBeNull() + }) + it('says partial when the sweep ran out of budget', async () => { vi.useFakeTimers({ shouldAdvanceTime: true }) const { toast } = await import('sonner')