Repository navigation
Stop the ARP read blocking the runtime, and the release check refiring - #196
Merged
Merged
Conversation
Two defects the independent reviews found. Both degrade the product in normal use and neither is visible from a passing test suite. 1. Reading the ARP table blocked tokio workers. On Windows and macOS it shells out to `arp` and waits on the child process. `Ops::get_arp_table` was the one sync method on an otherwise async facade, which invited exactly the bug it produced: three separate callers -- the MCP tool dispatcher, the Tauri command, and `DiscoverEngine` -- all invoked it straight from an async context. The MCP case was the sharp one. Handlers are capped at 16 concurrent and the stdin read loop is itself a task on the same pool, so sixteen `get_arp_table` calls could park every worker. No further request would be parsed -- including a cancellation. That is the same class of defect the concurrency rewrite fixed, at a call site it missed. Fixed at the source rather than per caller: `Ops::get_arp_table` is now async and moves the read to `spawn_blocking`, so every consumer benefits and the next one cannot repeat it. `DiscoverEngine` calls `NetworkManager::get_arp_table` directly, so that one is wrapped in place -- it was blocking a worker on every discover and sweep, not just on the ARP tool. This makes `Ops::get_arp_table` async, which is a breaking change for netscli-core consumers. It lands in the unreleased 0.3.0, and `Ops` is the async facade -- the sync signature was the anomaly. 2. The desktop app rate-limited itself out of update checks. `useReleaseNotifications` listed `showUpdateToast` as an effect dependency. It is a plain function from `useWorkspaceToast`, so every render gives it a new identity and the effect re-ran on every render: abort the in-flight request, issue another. During a scan that is roughly one request to api.github.com per progress event -- per probed port -- against an unauthenticated limit of 60 an hour. Idle, it still refired every 30s because the interface poll sets state with freshly deserialized values. The callbacks now live in a ref updated by a no-dependency effect, the same shape as the handler ref in useKeyboardShortcuts, so the check runs once per (appVersion, enabled) change. Both new tests were confirmed to fail against the previous behaviour before being kept: the release test reports 13 calls where it expects 1. cargo fmt, clippy with and without pcap, 147 Rust tests, 100 GUI tests, lint 0 errors, GUI build, file-size guard -- all pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects the independent reviews found. Both degrade the product in normal use, and neither is visible from a passing test suite.
1. Reading the ARP table blocked tokio workers
On Windows and macOS it shells out to
arpand waits on the child process.Ops::get_arp_tablewas the one sync method on an otherwise async facade, which invited exactly the bug it produced — three separate callers all invoked it straight from an async context:DiscoverEngineThe MCP case is the sharp one. Handlers are capped at 16 concurrent and the stdin read loop is itself a task on the same pool, so sixteen
get_arp_tablecalls could park every worker. No further request would be parsed — including a cancellation. That's the same class of defect the concurrency rewrite fixed, at a call site it missed.Fixed at the source rather than per caller:
Ops::get_arp_tableis now async and moves the read tospawn_blocking, so every consumer benefits and the next one can't repeat it.DiscoverEnginecallsNetworkManager::get_arp_tabledirectly, so that one is wrapped in place.Breaking change, deliberately.
Ops::get_arp_tablebecoming async breaks netscli-core consumers. It lands in the unreleased 0.3.0, andOpsis the async facade — the sync signature was the anomaly.2. The desktop app rate-limited itself out of update checks
useReleaseNotificationslistedshowUpdateToastas an effect dependency. It's a plain function fromuseWorkspaceToast, so every render gives it a new identity and the effect re-ran on every render: abort the in-flight request, issue another.During a scan that's roughly one request to api.github.com per progress event — per probed port — against an unauthenticated limit of 60 an hour. Idle, it still refired every 30s, because the interface poll sets state with freshly deserialized values.
The callbacks now live in a ref updated by a no-dependency effect — the same shape as the handler ref in
useKeyboardShortcuts— so the check runs once per(appVersion, enabled)change.Tests
Both were confirmed to fail against the previous behaviour before being kept:
Verification
cargo fmt, clippy with and withoutpcap, 147 Rust tests, 100 GUI tests, lint 0 errors, GUI build, file-size guard — all pass.Not verifiable here: the blocking fix is structural (the call moves to a blocking pool) and is covered by the type system and review, not by a test that measures runtime stall. Reproducing the original wedge needs a live MCP session with 16 concurrent ARP calls on a Windows or macOS host.