fix(speedtest): pin parallel stream count instead of following host CPU count - #261
fix(speedtest): pin parallel stream count instead of following host CPU count#261s0up4200 wants to merge 4 commits into
Conversation
…PU count speedtest-go takes its parallel stream count from runtime.NumCPU(), so the result depended on the host and not on the line. A 4-core box measured about half the upload of a 16-core box on the same connection. Pin the count so every host runs the same test. Also break the frequent distance ties on measured latency, and serialise tests so an overlapping scheduled and manual run stop corrupting the shared speedtest-go client.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe speedtest runner now uses 16 connections, selects nearby servers by distance and latency, serializes test execution, and resets client state after each test. Tests cover cancellation and server selection. ChangesSpeedtest selection and lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/speedtest/speedtest_net.go (1)
143-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn an error when no test server is selected.
selectNearestServerreturnsnilfor an empty server list, andRunTestdereferencesselectedServerwhen logging the server name. GuardselectedServer == nilafter auto-selection and return an error instead of panicking. Add aRunTestregression case for an empty server list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/speedtest/speedtest_net.go` around lines 143 - 148, Update RunTest’s auto-selection branch around selectNearestServer so it checks selectedServer again after selecting from serverList and returns a descriptive error when no server is available, before any dereference or logging. Add a RunTest regression case covering an empty server list and asserting the error result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/speedtest/speedtest_net.go`:
- Line 55: Add a docs/ entry describing that the speedtest client’s
MaxConnections setting is fixed at 16 and explain how this fixed stream count
affects measurement results. Reference the speedtest configuration using the
existing MaxConnections setting and keep the documentation focused on this
behavior change.
- Around line 95-99: Update RunTest’s admission gate to wait on a context-aware
semaphore or equivalent instead of blocking on r.mu.Lock(); check ctx
cancellation before acquiring the gate and return immediately when cancelled.
Preserve serialized access to r.client, and ensure cancelled callers do not
reset or otherwise touch r.client.
---
Outside diff comments:
In `@internal/speedtest/speedtest_net.go`:
- Around line 143-148: Update RunTest’s auto-selection branch around
selectNearestServer so it checks selectedServer again after selecting from
serverList and returns a descriptive error when no server is available, before
any dereference or logging. Add a RunTest regression case covering an empty
server list and asserting the error result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 922142b7-8342-4336-bc50-4db63921027b
📒 Files selected for processing (2)
internal/speedtest/speedtest_net.gointernal/speedtest/speedtest_net_test.go
Blocking on a plain mutex meant a request whose deadline expired while it waited still started a full test once the lock came free. Queue on a context-aware slot instead, and guard the empty server list that would otherwise panic when logging the selected server.
|
Thanks — two of the three were real and are fixed in 71d1302. Context-aware admission (major): correct, and it was introduced by this PR. The mutex made a request that had already timed out start a full test once the lock came free. Nil server (major): correct in shape. Docs entry (minor): declined. There is no configuration change here; the stream count is a fixed constant with no setting, so there is nothing for a user to set or read. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/speedtest/speedtest_net_test.go`:
- Around line 20-30: Update RunTest to recheck ctx after acquiring the running
slot and before FetchServers, returning context.Canceled when cancellation
occurred while queued; add a regression test that queues behind the occupied
slot, cancels the context, releases the slot, and verifies the canceled error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31929e14-5654-438e-9e10-be47265cab51
📒 Files selected for processing (2)
internal/speedtest/speedtest_net.gointernal/speedtest/speedtest_net_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/speedtest/speedtest_net.go
When the slot frees, both select cases are ready at once and Go picks one at random, so a caller that gave up while queued could still start a test half the time. Recheck the context after the slot is taken, before touching the client.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/speedtest/speedtest_net_test.go`:
- Around line 46-48: Replace the fixed time.Sleep in the RunTest test with a
deterministic synchronization hook or controlled slot-acquisition primitive that
signals once RunTest is blocked waiting on r.running; wait for that signal
before invoking cancel(), then release the slot and preserve the existing
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dc2ca3e-c580-4358-8c38-160cdc175962
📒 Files selected for processing (2)
internal/speedtest/speedtest_net.gointernal/speedtest/speedtest_net_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/speedtest/speedtest_net.go
The queued-caller test never reached the recheck. Cancelling before the slot frees wakes the parked goroutine on ctx.Done() every time, so the branch under test never ran and the test passed with the recheck removed. Cancel with the slot free instead: both cases are then ready when the select is evaluated and Go picks at random, so repeating it reaches the branch.
1 participant
speedtest-go takes its parallel stream count from
runtime.NumCPU(). The result therefore depended on the host, not on the line. Users with 4-core boxes reported speeds well below the Ookla app on the same connection. Upload suffered most. On a path where one TCP stream cannot fill the line, 4 streams measured 179 Mbps upload where 24 measured 995. On a fast, low-latency path the count made no difference: 4 and 24 streams both gave 875 Mbps upload, against 889 Mbps from iperf3. A pinned count of 16 recovers a lot on a poor path and changes nothing on a good one. It also costs almost no CPU — a container limited to one core still reached line rate with 24 streams.Two smaller faults go with it. Ookla reports the same distance for every server in a city, so choosing "the nearest" resolved an arbitrary tie: the lucky server gave 710 Mbps and the unlucky one 587. The choice now goes to the lowest measured latency among the closest servers. The runner also shared one speedtest-go client across all tests with no lock, so a scheduled test that overlapped a manual one recorded 0 Mbps. Tests now queue. Stored history will show a step up after this change, mostly in upload, and mostly on hosts with fewer than eight cores.
Summary by CodeRabbit
Improvements
Tests