feat: NetMonitor v2.2.1 reliability and responsiveness - #265
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the v2.2.1 “Reliability & Responsiveness” proposal (#262) across the shared Swift packages and both app targets, focusing on cancellation safety, deterministic lifecycle behavior, and explicit persistence recovery.
Changes:
- Makes scan execution more cancellation-aware (phase cancellation checks, phase deadlines/timeouts, connection budget cancellation) and adds regression tests.
- Hardens companion transport framing and reconnect behavior on iOS (actor-isolated frame decoder, generation IDs, exponential backoff) with updated tests.
- Replaces silent/implicit persistence fallback behavior with explicit degraded-mode recovery UI + diagnostic export and store compatibility tests.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/NetMonitor-macOSTests/DeviceDiscoveryCoordinatorTests.swift | Adds regression coverage for merging devices when historical identifiers contain duplicates. |
| Tests/NetMonitor-macOSTests/AppearanceModeTests.swift | Adds “last shipped store” reopen/migration-plan test coverage for macOS. |
| Tests/NetMonitor-iOSTests/MacConnectionServiceTests.swift | Updates frame-processing tests for async decoder and adds reconnect backoff policy test. |
| Tests/NetMonitor-iOSTests/MacConnectionServiceReceiveBufferTests.swift | Updates receive-buffer tests for async frame decoding. |
| Tests/NetMonitor-iOSTests/EventListenerServiceTests.swift | Adds cancellation regression test for observation-wait bridging. |
| Tests/NetMonitor-iOSTests/BackgroundTaskServiceExtendedTests.swift | Improves background task registration tests (idempotency) and sandbox behavior. |
| Packages/NetworkScanKit/Tests/NetworkScanKitTests/ScanEngineTests.swift | Adds cancellation and phase-timeout regression tests for scan pipeline execution. |
| Packages/NetworkScanKit/Tests/NetworkScanKitTests/ConnectionBudgetTests.swift | Adds waiter cancellation/reset regressions and tightens acquire/release assertions. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/ScanEngine.swift | Introduces per-phase timeout racing and cancellation checks between phases. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/Phases/TCPProbeScanPhase.swift | Adds cancellation checks and budget-acquire handling around high-concurrency probing. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/Phases/SSDPScanPhase.swift | Adds cancellation gates and budget-acquire handling in SSDP discovery. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/Phases/ReverseDNSScanPhase.swift | Adds cancellation handling during concurrent reverse DNS resolution. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/Phases/ICMPLatencyPhase.swift | Adds cancellation gates around latency measurement and accumulator updates. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/Phases/BonjourScanPhase.swift | Adds cancellation handling (including provider stop) across discovery and resolution loops. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/Phases/ARPScanPhase.swift | Adds cancellation gates around ARP cache population/sleep/read/update. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/NWConnectionHelper.swift | Makes NWConnection state awaiting cancellation-safe and timeout-safe via actor state. |
| Packages/NetworkScanKit/Sources/NetworkScanKit/ConnectionBudget.swift | Adds cancellation-aware waiters and exposes active/waiting counts for diagnostics/tests. |
| Packages/NetMonitorCore/Tests/NetMonitorCoreTests/ScanProgressCoalescerTests.swift | New tests for deterministic progress throttling/coalescing behavior. |
| Packages/NetMonitorCore/Tests/NetMonitorCoreTests/PersistenceBootstrapTests.swift | New tests ensuring persistent-store failure is explicit and fallback behavior is visible. |
| Packages/NetMonitorCore/Tests/NetMonitorCoreTests/LastShippedStoreCompatibilityTests.swift | New “v2.2.0 store reopens” compatibility regression test for iOS schema store. |
| Packages/NetMonitorCore/Tests/NetMonitorCoreTests/CompanionFrameDecoderTests.swift | New tests for fragmentation, adjacency, and oversized-frame rejection behavior. |
| Packages/NetMonitorCore/Sources/NetMonitorCore/Services/PortScannerService.swift | Updates to respect connection budget acquisition failure (don’t proceed without a slot). |
| Packages/NetMonitorCore/Sources/NetMonitorCore/Services/PingService.swift | Updates to respect connection budget acquisition failure in multi-port probing. |
| Packages/NetMonitorCore/Sources/NetMonitorCore/Services/DeviceDiscoveryService.swift | Adds progress coalescing, stronger cancellation guards, and ensures Bonjour discovery stops. |
| Packages/NetMonitorCore/Sources/NetMonitorCore/Services/BonjourDiscoveryService.swift | Updates service resolution to respect connection budget acquisition failure. |
| Packages/NetMonitorCore/Sources/NetMonitorCore/ScanProgressCoalescer.swift | Adds coalescer for limiting scan progress UI amplification (default 10 Hz). |
| Packages/NetMonitorCore/Sources/NetMonitorCore/PersistenceBootstrap.swift | Adds explicit persistence bootstrap outcome + recovery state for degraded-mode UI. |
| Packages/NetMonitorCore/Sources/NetMonitorCore/CompanionFrameDecoder.swift | Adds an actor-isolated length-prefixed framing decoder with bounded frames and reset. |
| NetMonitor-macOS/Views/Tools/PortScannerToolView.swift | Makes port checks cancellation-aware and refactors timeout handling structure. |
| NetMonitor-macOS/Views/DevicesView.swift | Bounds device fetching to profile-scoped results and adds refreshed reload triggers + IDs. |
| NetMonitor-macOS/Views/Components/PersistenceRecoveryView.swift | Adds macOS degraded-mode recovery UI with diagnostic export affordance. |
| NetMonitor-macOS/Utilities/LocalDeviceQueries.swift | Removes unbounded “all devices” fetch; emphasizes capped/predicate-backed descriptors. |
| NetMonitor-macOS/Platform/ISPLookupService.swift | Removes inline SwiftLint suppression comments around force unwraps. |
| NetMonitor-macOS/Platform/DeviceDiscoveryCoordinator.swift | Batches merge lookups (MAC/IP dictionaries) to avoid N+1 fetch patterns. |
| NetMonitor-macOS/App/NetMonitorApp.swift | Uses PersistenceBootstrap to surface recovery UI instead of silent in-memory fallback. |
| NetMonitor-iOS/Views/Tools/SpeedTestToolView.swift | Bounds SwiftData history query via fetchLimit to reduce UI load. |
| NetMonitor-iOS/Views/Settings/SettingsView.swift | Avoids eager @Query table loads; fetches on-demand for export paths. |
| NetMonitor-iOS/Views/Dashboard/DashboardView.swift | Improves scan/empty/offline dashboard states and tightens accessibility identifiers. |
| NetMonitor-iOS/Views/Components/PersistenceRecoveryView.swift | Adds iOS degraded-mode recovery UI with shareable diagnostic export. |
| NetMonitor-iOS/ViewModels/DashboardViewModel.swift | Exposes scan progress/phase for updated dashboard state rendering. |
| NetMonitor-iOS/Platform/MacConnectionService.swift | Moves framing into CompanionFrameDecoder actor, adds stale-callback protection + backoff. |
| NetMonitor-iOS/Platform/EventListenerService.swift | Adds cancellation-safe observation bridging to avoid suspended continuations. |
| NetMonitor-iOS/Platform/BackgroundTaskService.swift | Adds injected registration hook + idempotent registration guard and expiration stop-scan. |
| NetMonitor-iOS/App/NetmonitorApp.swift | Registers background tasks at launch (idempotent) and surfaces persistence recovery UI. |
| NetMonitor-2.0.xcodeproj/project.pbxproj | Wires new recovery views into build phases/groups and updates project structure. |
Suppressed comments (1)
NetMonitor-macOS/Platform/DeviceDiscoveryCoordinator.swift:174
- When a discovered device matches an existing one,
existing.ipAddressis updated butdevicesByIPis not. If multipledevicesare merged in one call (or a subsequent discovered device uses the previous IP), the dictionary can point at the wrongLocalDevice, leading to incorrect updates/merges.
if let existing {
existing.ipAddress = discovered.ipAddress
if let hostname = discovered.hostname, !hostname.isEmpty {
existing.hostname = hostname
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public func release() { | ||
| active = max(active - 1, 0) | ||
| if !waiters.isEmpty { | ||
| while !waiters.isEmpty { | ||
| let next = waiters.removeFirst() | ||
| next.resume() | ||
| guard !next.state.isCancelled else { | ||
| next.continuation.resume(returning: false) |
| private func reloadDevices() { | ||
| devices = (try? modelContext.fetch(LocalDeviceQueries.forProfile(activeProfileID))) ?? [] | ||
| } |
| } | ||
| } | ||
|
|
||
| connection.start(queue: DispatchQueue(label: "com.netmonitor.port-scanner")) |
| guard !hasRegisteredTasks else { | ||
| Self.logger.debug("Background tasks already registered; skipping duplicate request") | ||
| return | ||
| } | ||
| hasRegisteredTasks = true |
| /// Run an async operation with a timeout. If the operation exceeds the | ||
| /// timeout, its task is cancelled and we move on. |
| let service = MacConnectionService.shared | ||
| service.disconnect() | ||
|
|
||
| // Frame length of 0xFF_FF_FF_FF (>10MB limit) |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| break | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Port scanning creates a brand-new background work queue for every port, risking thread exhaustion
Each port probe now runs on its own freshly created dispatch queue (DispatchQueue(label: "com.netmonitor.port-scanner") at NetMonitor-macOS/Views/Tools/PortScannerToolView.swift:370) instead of the shared one, so a large scan spawns hundreds of queues and can starve the system thread pool.
Impact: Scanning many ports can make the Mac app sluggish or hang instead of returning results.
Per-connection queue creation replaces the shared global queue
The previous implementation used connection.start(queue: .global()). The new code constructs a distinct serial DispatchQueue per checkPort(host:port:) invocation. scanPorts runs 50 probes concurrently per batch (NetMonitor-macOS/Views/Tools/PortScannerToolView.swift:292-320), so at least 50 live serial queues exist simultaneously, each capable of pulling a distinct GCD worker thread while NWConnection callbacks are delivered; a 1024-port custom range creates 1024 queues over the scan. This is the same anti-pattern that was explicitly removed elsewhere in the codebase ("perf(scan): reuse shared queue in quickPortScan (#203)").
| connection.start(queue: .global()) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| guard connectionGeneration == generation else { return } | ||
| heartbeatTask?.cancel() | ||
| heartbeatTask = nil | ||
| connection = nil |
There was a problem hiding this comment.
🟡 Failed companion connections are discarded without being shut down, leaking network resources
When the link to the Mac drops, the underlying connection object is forgotten (connection = nil at NetMonitor-iOS/Platform/MacConnectionService.swift:319) without ever being shut down, so its sockets and callbacks stay alive for the rest of the app session.
Impact: Repeated reconnect cycles accumulate abandoned network connections, wasting memory and system resources on the phone.
Reference dropped before cancel(), and disconnect() can no longer reach it
Both new terminal paths clear the stored reference without cancelling:
handleReceiveTermination(error:generation:)setsconnection = nil(NetMonitor-iOS/Platform/MacConnectionService.swift:319) afterisComplete/error..failedinhandleConnectionStatesetsself.connection = nil(NetMonitor-iOS/Platform/MacConnectionService.swift:234).
Before this PR the object stayed in connection so a later disconnect() (NetMonitor-iOS/Platform/MacConnectionService.swift:181) would call cancel() on it. Now disconnect() sees nil. NWConnection requires an explicit cancel() to release its underlying resources; a failed connection that is never cancelled is leaked. Each auto-reconnect attempt that fails adds another leaked connection.
Prompt for agents
In NetMonitor-iOS/Platform/MacConnectionService.swift, both new terminal paths drop the NWConnection reference without cancelling it: handleReceiveTermination sets `connection = nil` and the `.failed` branch of handleConnectionState does the same. NWConnection must be explicitly cancelled to release its resources, and disconnect() can no longer do it once the reference is nil. Cancel the connection (either the local `connection` parameter passed into scheduleReceive/handleConnectionState, or the stored property) before clearing the reference in both paths.
Was this helpful? React with 👍 or 👎 to provide feedback.
| TextField("Search...", text: $searchText) | ||
| .textFieldStyle(.plain) | ||
| .font(.system(size: 12)) | ||
| .accessibilityIdentifier("devices_textfield_search") |
There was a problem hiding this comment.
🟡 Device search field on Mac now carries the same automation identifier as its surrounding container
The new identifier added to the search text field (accessibilityIdentifier("devices_textfield_search") at NetMonitor-macOS/Views/DevicesView.swift:427) duplicates the one already set on the enclosing row, so anything looking that element up by name now finds two matches.
Impact: Automated checks and accessibility tooling can resolve the wrong element or fail outright when locating the device search box.
Duplicate identifier on nested elements
The container HStack already declares .accessibilityIdentifier("devices_textfield_search") at NetMonitor-macOS/Views/DevicesView.swift:445. Adding the same string to the inner TextField produces two elements in the accessibility hierarchy with the identical identifier. Existing UI tests reference it (Tests/NetMonitor-macOSUITests/EdgeCasesAndErrorStatesUITests.swift:226, Tests/NetMonitor-macOSUITests/SidebarSelectionContentUITests.swift:43), and XCUIElement queries that resolve to multiple matches raise a "multiple matches" failure when accessed directly.
Prompt for agents
NetMonitor-macOS/Views/DevicesView.swift now sets accessibilityIdentifier("devices_textfield_search") on both the TextField (line ~427) and the wrapping HStack (line ~445). Keep exactly one of them so element lookups are unambiguous — most likely keep it on the TextField and remove it from the container, then confirm the macOS UI tests that reference the identifier still target the right element.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let diagnosticText = """ | ||
| NetMonitor persistent store recovery | ||
| Timestamp: \(Date().formatted(.iso8601)) | ||
| Error: \(String(reflecting: persistentStoreError)) | ||
| """ |
There was a problem hiding this comment.
🟨 Persistence diagnostics bundle embeds raw store error, which may expose local file paths and store contents
PersistenceBootstrap.load builds diagnosticText from String(reflecting: persistentStoreError) (Packages/NetMonitorCore/Sources/NetMonitorCore/PersistenceBootstrap.swift:43-47). SwiftData/CoreData errors commonly embed the full on-disk store URL (which includes the user account name and container path) and sometimes fragments of the offending model/row data. That text is surfaced verbatim through ShareLink(item: recovery.diagnosticText) in both NetMonitor-iOS/Views/Components/PersistenceRecoveryView.swift:13 and NetMonitor-macOS/Views/Components/PersistenceRecoveryView.swift:13, so a user can unknowingly forward environment details to a third party.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Implements the v2.2.1 Reliability and Responsiveness release proposal tracked in #262.
Verification
Release-candidate blockers
Tracks #262.