Repository navigation
Add --nearest flag to measure latency and auto-select closest cluster - #2571
Merged
Merged
Conversation
A repo readable from several clusters was offered in alphabetical host order, so aws-ap-southeast-2 outranked us-east-1 on the letter `a`, and a non-interactive caller got an error instead of a default. A user who cloned from the far placement kept it: the host is written into .git/config at clone time and every later fetch pays the distance. `repo clone` now dials each candidate cluster concurrently under one 400ms budget, offers them nearest first in the picker, and defaults to a placement that is clearly nearer when there is no terminal to prompt. The measurement is client-side. The CLI times its own TCP connects, so nothing is sent about where the caller is and no location is inferred from an IP address. Two rules keep the default honest. A candidate must beat the incumbent by 25ms before it is chosen without a human, because a mirror trails its primary and a few milliseconds do not pay for that staleness. Any unmeasured host loses its position in the ordering but keeps its place in the list, because one unanswered probe does not make a placement bad. With no measurement at all the picker is exactly what it was before. The incumbent is currently the first host in alphabetical order, which is a placeholder: the /gh/ path has no primary in the coreapi shape, whose ResolvedPlacement carries no role. publicv1 already models one (RoleProcessingPrimary), so the incumbent should become the announced primary once that field reaches this client. The probe is a field on placementPicker rather than a package var, so tests substitute one without shared state and stay parallel. Verification: gofmt, go build, go test -race ./... — the placement tests pass and the remaining failures match the pre-change baseline on this machine. `mise run lint` NOT run: the pinned golangci-lint 2.13.2 is unavailable here and the installed build refuses the repo's Go version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
Nearest-placement selection now runs only under `repo clone --nearest`. Without the flag the picker has no probe, dials nothing, and orders placements alphabetically exactly as before. Opting in removes the reason for the 25ms margin the first version carried, so the margin is gone and the nearest measured placement wins outright. The margin guarded against silently trading a primary for a mirror that trails it. A user who types --nearest and is handed the far placement has instead been overruled by a rule they cannot see — and the "incumbent" the margin compared against was the alphabetically first host, which is not the primary and means nothing. A staleness guard belongs here only once the primary is identifiable (publicv1 models the role, coreapi does not yet carry it) and only if this becomes the default. --nearest and --cluster are refused together. Both name the cluster to clone from and they disagree whenever --cluster is not already the nearest, so ranking them would silently pick the remote URL the user keeps. A chosen placement is announced on stderr with its round trip and the --cluster override. The host lands in .git/config and every later fetch follows it, so a selection nobody saw is one nobody can question. Long help and examples cover the flag, including that the measurement is local and sends nothing about where the caller is. Verification: gofmt, go vet, go build, go test -race ./cmd/entire/cli/... — placement tests pass, remaining failures match the pre-change baseline on this machine. `mise run lint` NOT run: the pinned golangci-lint 2.13.2 is unavailable here and the installed build refuses the repo's Go version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved issues remain in probing, URL handling, and privacy documentation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds an opt-in --nearest flag to repo clone, using concurrent TCP latency probes to select and display the fastest cluster.
Changes:
- Adds latency probing, ordering, and RTT formatting.
- Integrates nearest-cluster selection and interactive display behavior.
- Adds flag, placement, and latency tests.
| File | Summary |
|---|---|
cmd/entire/cli/repo_clone.go |
Integrates the flag and placement selection behavior. |
cmd/entire/cli/repo_clone_test.go |
Tests nearest selection, messaging, and flag behavior. |
cmd/entire/cli/placement_latency.go |
Implements concurrent latency probing and ordering. |
cmd/entire/cli/placement_latency_test.go |
Tests latency ordering and formatting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
main reworked selectPlacement in parallel with this branch: it now takes
a defaultHost and resolves the repo's primary cluster where there is no
terminal, instead of refusing. The caller supplies it per forge — a
native repo's home cluster from the repo itself, defaultClusterHost for
a GitHub mirror set.
That overlaps with --nearest, which existed to replace the same refusal.
The resolution composes the two rather than picking a side:
- --nearest substitutes the measured nearest placement for the primary
as the no-terminal default. Displacing it is what the flag asks for.
- An unmeasurable network now falls through to the primary instead of
the error this branch used to end at. A caller who opted in should not
land somewhere worse than one who did not.
- The announcement names the primary it replaced ("not the primary
<host>") and stays silent when the nearest IS the primary. A mirror
lags its primary, so trading one for the other is the part a reader
most needs to see; nothing traded means nothing to warn about.
This also retires a limitation recorded on this branch: the primary was
said to be unidentifiable because coreapi.ResolvedPlacement carries no
role. defaultHost supplies it, so a staleness margin over the primary is
now implementable if --nearest ever becomes the default.
Verification: gofmt, go vet, go build, go test -race ./cmd/entire/cli/...
The failures match a run of origin/main in a clean worktree exactly, so
the merge adds none.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Two review findings and the two lint failures on PR #2571. probeAddress replaces the unconditional ":443" suffix. A placement host may already carry a port — validateClusterHost admits a bare "host[:port]" and hostFromPublicURL preserves what the registry published — so a cluster on host:8080 became "[host:8080]:443", an address that never resolves. The probe reported it unreachable and --nearest ranked it last or dropped it, while `git clone` reached it fine. The port dialled is now the port the clone uses. IPv6 needs both halves: "[::1]:8080" is returned as is, and a bare "::1" is bracketed once instead of twice. dialLatencies had no test at all: every selection test injects a stub, so address construction, the fan-out and connection cleanup were unexercised. TestDialLatencies probes loopback listeners it owns, so it needs no network and no name resolution, and covers a reachable host, a host that refuses the connection, a cancelled context standing in for the budget expiring, and an empty host list. Both new tests fail against the previous probeAddress, which is what makes them a regression guard rather than description. Lint: the //nolint:errcheck on the probe socket close was unused, since "_ =" already satisfies errcheck (nolintlint), and the argument-free fmt.Errorf for the flag conflict becomes errors.New (perfsprint). The trail's findings gate passed with zero findings; its confidence monitor named the untested dial path, which is what TestDialLatencies closes. Verification: gofmt -s, go vet, go build, go test -race ./cmd/entire/cli/... — failures match a clean origin/main worktree run exactly. golangci-lint still cannot run in this container, so the two lint fixes are verified by inspection against the CI log, not by a local run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
Member
Author
|
@cursor review |
noctx rejects net.Listen, which takes no context. listenLoopback binds the listen to the test's context instead, so a cancelled test tears its listener down rather than leaking it to the cleanup. The helper returns the listener alone: require.NoError already fails the test, so an error return had no caller that could act on it. Verification: gofmt -s, go vet, go build, go test -race ./cmd/entire/cli/... — failure set matches a clean origin/main worktree run. golangci-lint still cannot run in this container, so this fix targets the exact noctx diagnostic from the CI log. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
Member
Author
|
@cursor review |
A placement still connecting when the probe budget ran out was dropped from the map and shown unlabelled, next to placements reading "18ms". A reader could not tell it from one the probe could not measure, which is the comparison the picker exists to support. It now prints ">400ms": the exact figure is unknown, the lower bound is not, and the bound is formatted from placementProbeBudget so the two cannot drift. A timeout and a refusal are now distinct outcomes rather than one "absent". Only a deadline means slow: a refused connection or a failed resolution comes back immediately and is no evidence of distance, so labelling it ">400ms" would report a latency never observed and make a firewalled port look like the far side of the planet. Cancellation is the caller giving up and stays absent too. That splits the ordering into three tiers — answered, ascending; timed out; unmeasurable — each still offered, since a dropped SYN, a paused laptop and a corporate proxy produce the same silence. A timed-out host ranks but never wins: ">400ms" is enough for a person weighing the picker, not enough to choose unattended, because two timed-out placements are indistinguishable and the primary is a better answer than either. So --nearest with nothing but timeouts still falls through to the primary. The new dial test uses an already-passed deadline as a deterministic stand-in for the budget expiring mid-dial, so the timeout classification is covered without a hanging host or a 400ms wait. Verification: gofmt -s, go vet, go build, go test -race ./cmd/entire/cli/... — failure set matches a clean origin/main worktree run. golangci-lint still cannot run in this container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
Three trail findings, plus the wording half of a fourth. nearestHost now takes the incumbent — the repo's primary — and answers only when the primary itself produced a measurement and something beat it. A primary that dropped a single probe used to lose to a mirror measured at 300ms, which was then announced as "nearest" and written into .git/config on the strength of a probe the primary never answered. That contradicted the reasoning already in this file: orderHostsByLatency argues a placement missing a probe is silent rather than bad, "so it loses its position in the ordering, not its place in the list", and selection then made the same silence decisive. Comparing two numbers requires two numbers. A timed-out host is not a measurement on either side of the comparison: ">400ms" ranks a host in the picker but cannot win or lose one unattended. The probe no longer dials whatever the API returned. validateClusterHost gates the CHOSEN placement as "defense-in-depth against a malformed host reaching git", and dialling first walked in front of it: an empty ClusterHost dials ":443", which is the local machine, and probeAddress honours an embedded port so the target is not pinned to 443 either. Hosts are validated before they are connected to, and a rejected host is dropped from the probe but stays in the picker, since it is refused after selection anyway. The privacy claim was too strong. "Sends nothing about where you are" holds for the control plane and not for the clusters: a TCP SYN tells the cluster it reaches the caller's source IP. Without the flag one cluster learns that, the one being cloned from; with it, every candidate does. The help and the package comment now say both halves. Nothing user-facing calls the measurement a round trip any more. DialContext resolves inside the timed region, so the figure is connect time — a cold resolver cache is counted, and a host resolved earlier in the same command carries a systematic advantage. Comparable enough to rank placements, not a latency benchmark. Narrowing the measurement (resolving outside the timing, or best-of-two) changes what the budget covers and is left for a follow-up. The announcement now prints both figures — chosen and primary — since reaching it guarantees both exist. Verification: gofmt -s, go vet, go build, go test -race ./cmd/entire/cli/... — failure set matches a clean origin/main worktree run. Both behavioural fixes were mutation-checked: reverting either fails the tests added here. golangci-lint still cannot run in this container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
Member
Author
|
@cursor review |
pjbgf
enabled auto-merge
September 23, 2026 15:31
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3656b5d. Configure here.
gtrrz-victor
approved these changes
Sep 24, 2026
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.
https://entire.io/gh/entireio/cli/trails/1411
Summary
Adds a
--nearestflag torepo clonethat measures round-trip latency to each available cluster and automatically selects the fastest one, making multi-cluster repositories scriptable without requiring--clusteror an interactive terminal.Changes
New latency measurement module (
placement_latency.go): Implements concurrent TCP dial probing to measure round-trip time to candidate clusters within a 400ms budget. Includes helper functions to order hosts by measured latency and select the nearest one.--nearestflag forrepo clone: When passed, installs a latency probe into the placement picker. The probe is opt-in and entirely client-side—no information about the caller's location is sent to the server.Automatic placement selection: When multiple placements are available and no terminal is present:
--nearest: Selects the fastest measured cluster and announces the choice on stderr--nearest: Errors as before, requiring--clusterto be specifiedMutual exclusivity:
--nearestand--clustercannot be used together, since they both choose a cluster and may disagree.Placement picker UI enhancement: Measured round-trip times are appended to placement labels in the interactive picker (e.g.,
"aws-us-east-2 — aws-us-east-2.entire.io [18ms]"), making the latency-based ordering visible to users.Comprehensive test coverage: Adds unit tests for latency ordering, nearest-host selection, and RTT formatting. Integration tests verify end-to-end behavior including the scriptability win, probe failure handling, and announcement messaging.
Implementation details
placementPickerand defaults tonil, ensuring the default clone path dials nothing and preserves existing behavior.--clustervalue to pass if they want to override it.https://claude.ai/code/session_015fehoxAU4keeBvQMHfKCtv
Note
Medium Risk
Opt-in flag changes which cluster host lands in clone URLs and
.git/config, and probes every candidate (client-side TCP), though invalid hosts are filtered and failed probes fall back to the primary.Overview
Adds opt-in
--nearestonentire repo clone: when a repo is readable on multiple clusters, the CLI can TCP-probe candidates (400ms shared budget, default port 443 with correct host:port handling) and rank placements by connect time instead of alphabetical order.Placement selection wires an injectable
latencyProbeonplacementPicker(only when the flag is set).selectPlacementprobes aftervalidateClusterHostfiltering, reorders the interactive picker (labels show timings like[18ms]or>400ms), and in non-interactive runs can auto-pick the fastest only if the primary was measured and beaten—otherwise it keeps the primary, including when probes fail or the primary was unmeasured (regression fix). Swapping primary for a mirror is announced on stderr;--nearestand--clusterare rejected together.New
placement_latency.goholds ordering, nearest-host logic, and dial helpers; tests cover unit behavior and clone integration via stub probes.Reviewed by Cursor Bugbot for commit 3656b5d. Configure here.