Add targeted internet speed test - #50
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Final review identified a critical payload-bounds issue and multiple moderate functional, cancellation, and lifecycle issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a Windows asynchronous internet speed-test API and popup meter integrated with gateway selection and tray routing.
Changes:
- Adds portable speed-test results and quality grading.
- Implements asynchronous WinHTTP latency, download, and upload measurements.
- Integrates gateway selection, meter UI, cancellation, and build targets.
File summaries
| File | Summary and final findings |
|---|---|
src/ui/tray_win32.c |
Integrates popup routing. Critical, 2 votes: WM_COPYDATA handling can read past an unterminated payload; validate bounds and require an in-range terminator. |
src/ui/speed_meter_win32.h |
Declares the meter API. Moderate, 1 vote: showing the meter does not start the fresh measurement promised by the contract. |
src/ui/speed_meter_win32.c |
Implements the meter and worker lifecycle. Moderate, 2 votes: destruction can block the UI thread during network calls. Moderate, 3 votes: replacing router metadata can leave an old worker and progress snapshot active. |
src/ui/network_map_win32.h |
Adds router-selection messaging. |
src/ui/network_map_win32.c |
Handles gateway-node selection. |
src/platform/win32/speedtest_win32.c |
Implements WinHTTP measurements. Moderate, 1 vote: failed or rejected upload responses can still mark uploads valid. Moderate, 2 votes: the selected router is not used for the connection. Moderate, 1 vote: download reads do not poll cancellation between chunks. Moderate, 1 vote: the upload loop does not poll cancellation between writes. Moderate, 1 vote: the final response wait is not skipped after cancellation. |
src/core/speedtest.c |
Implements quality grading. Nit, 2 votes: threshold, weakest-dimension, and partial/unknown-result cases lack test coverage. |
include/linkpulse/speedtest.h |
Defines the public speed-test API. |
CMakeLists.txt |
Adds the new core, platform, and UI sources to the build. |
Review details
Suppressed comments (5)
src/platform/win32/speedtest_win32.c:327
- If
WinHttpReceiveResponsefails, or if it returns a non-200 status, this path still reaches the accounting below withtotal_bytes > 0and marks the upload as valid. That reports an upload speed even though the endpoint never accepted the request; treat either response failure or rejection asLP_ERR_IObefore settinghas_upload.
if (remaining == 0 && WinHttpReceiveResponse(response, NULL)) {
const DWORD http_status = response_status_code(response);
if (http_status != 200) {
LP_WARN("speed test upload rejected: http_status=%lu", http_status);
WinHttpCloseHandle(response);
src/platform/win32/speedtest_win32.c:211
- Cancellation is checked only by the outer loop. Once a 25 MB response is open, this inner loop can continue reading the entire body before observing
cancel_flag; on a slow link,stop_testcan therefore wait for the remainder of the transfer instead of stopping between chunks as the public API promises. Pollis_cancelled(request)before eachWinHttpReadDatacall.
for (;;) {
DWORD read = 0;
if (!WinHttpReadData(response, buffer, (DWORD)sizeof(buffer), &read)) {
src/platform/win32/speedtest_win32.c:323
- After the final upload chunk, this unconditional
WinHttpReceiveResponsecan still wait for the server even when cancellation was requested. Sincestop_testjoins this worker, backing out or shutting down can block unnecessarily; skip the response wait whencancel_flagis set.
if (remaining == 0 && WinHttpReceiveResponse(response, NULL)) {
src/platform/win32/speedtest_win32.c:301
- The upload loop only checks
cancel_flagin the outer loop. Once this 10 MB request is open, it can keep callingWinHttpWriteDatauntil the entire request is sent even after the user cancels, contrary to the API's between-chunk cancellation contract.
while (remaining > 0) {
const DWORD chunk = remaining < sizeof(buffer) ? (DWORD)remaining
: (DWORD)sizeof(buffer);
DWORD written = 0;
if (!WinHttpWriteData(response, buffer, chunk, &written) || written == 0) {
src/ui/speed_meter_win32.h:16
- This public contract says showing the meter starts a fresh measurement, but
lp_speed_meter_showonly updates the labels, displays the window, and invalidates it;start_testis called only from the Run test button handler. Opening a gateway therefore leaves the meter at “Ready to test” instead of starting the promised run. Either start the worker from the show path or revise the contract and UI flow to make the extra click explicit.
/* Shows the meter for one router and starts a fresh measurement run. */
void lp_speed_meter_show(HWND window, const char *router_label, const char *router_ip,
const char *isp, bool use_bits);
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Refactor IP address handling to ensure safe copying and null termination. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Boussetta <4375228+Boussetta@users.noreply.github.com>
Co-authored-by: Boussetta <4375228+Boussetta@users.noreply.github.com>
Co-authored-by: Boussetta <4375228+Boussetta@users.noreply.github.com>
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.
Summary
Validation
The speed-test work is based on merged PR #49 and is isolated from the per-network device-store changes.