fix(scan): keep an edit, a discovery source and one name for a failed run

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

@@ -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()
 
 

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

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"

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

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

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

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(
+      <InventoryDeviceModal {...noop} onSaved={onSaved} device={device} />
+    )
+    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(<InventoryDeviceModal {...noop} onSaved={onSaved} device={fresh} />)
+
+    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(<InventoryDeviceModal {...noop} device={makeDevice()} />)
+    fireEvent.click(screen.getByRole('button', { name: 'Edit' }))
+    fireEvent.change(screen.getByPlaceholderText('Display name'), { target: { value: 'Media box' } })
+
+    rerender(<InventoryDeviceModal {...noop} device={makeDevice({ id: 'dev-2' })} />)
+
+    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')