From a20a65da29d0c9f71694b57649f53d7f00744a19 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 2 Sep 2026 17:16:51 +0200 Subject: [PATCH 1/2] Fix async mirror-create review findings from #2207 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #2246 made the async route the default, these are live for everyone rather than gated behind an opt-in flag. Drives the placement poll off the 202 body's requestId instead of the Location header, which removes three problems at once: an optional header no longer fails a create whose body already reports success, no server-named host can become a poll target for the control-plane bearer, and the poll keeps 421ing to the home core so the transport's bare-401 re-exchange stays recoverable past the token cache. With nothing reading the header, the mirrorLocationCanonicalizer that maintained it goes too — it survived the auth-go/crossjuris refactor in 02abf214b, but its only consumer is gone. Restores parity between the two routes: MirrorRequestResult carries no `suspended` field, so an async create against a suspended placement reported plain success and exited 0 where the sync route warns and exits non-zero. One status read fills it in for every caller. Makes --wait-timeout cover submission, placement and clone on both routes. It previously bounded only the clone poll on the sync route, so a hanging CreateMirror ignored the flag; the help text documented that split rather than fixing it. Also: an accepted request's id survives a wait that ended with no verdict, so the user has a handle on in-flight work — but not a terminal failure, which is done and must not be described as still progressing; the wizard labels a placement timeout "timed out" rather than "error"; one relabelled spinner replaces three per-phase spinners that each stamped their own check mark; and ENTIRE_ASYNC_MIRROR_REQUESTS overrides the route in both directions, since this command usually runs outside the repo whose settings.json holds the opt-out. A settings-load error is now reported instead of swallowed, without changing the route — a read that failed is not evidence the repo opted out. Restores the poll-budget and createAndAwaitMirror rationale comments dropped in #2207, documents the new functions and the unreachable unknown-status branch, and records why the session-store FD test is deliberately not parallel. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M1VQ3ZN5Y7DVA4ZXSF0DVV9H --- cmd/entire/cli/agent/session_store_test.go | 10 +- cmd/entire/cli/repo_mirror.go | 187 +++++++++-- cmd/entire/cli/repo_mirror_create_wizard.go | 13 +- cmd/entire/cli/repo_mirror_probe.go | 130 +++++--- cmd/entire/cli/repo_mirror_request_test.go | 333 +++++++++++++++++++- cmd/entire/cli/repo_mirror_test.go | 9 +- internal/coreapi/cross_juris_client.go | 52 +-- internal/coreapi/cross_juris_client_test.go | 32 -- 8 files changed, 607 insertions(+), 159 deletions(-) diff --git a/cmd/entire/cli/agent/session_store_test.go b/cmd/entire/cli/agent/session_store_test.go index 6df76b2a28..12f737092a 100644 --- a/cmd/entire/cli/agent/session_store_test.go +++ b/cmd/entire/cli/agent/session_store_test.go @@ -141,6 +141,11 @@ func TestWriteSessionFile_RequiresASessionRef(t *testing.T) { // measured — so a few hundred project directories reached RLIMIT_NOFILE, and // because the registry is shared, os.OpenRoot then failed for .entire and the // git common dir too. +// +// Deliberately NOT parallel, against the repo default: the assertion is a +// before/after count of THIS PROCESS's open descriptors, which any concurrent +// test opening a file or a root perturbs. Restoring t.Parallel() here for +// convention's sake makes the count flaky rather than the test faster. func TestSessionStore_ProbingManyDirectoriesRetainsNoDescriptors(t *testing.T) { countFDs := func() int { entries, err := os.ReadDir("/proc/self/fd") @@ -166,8 +171,9 @@ func TestSessionStore_ProbingManyDirectoriesRetainsNoDescriptors(t *testing.T) { require.NoError(t, err) store.Exists(name) // absent in every candidate, as in a real miss } - // Some slack for anything the runtime opens concurrently; the regression - // this guards produced exactly `candidates` extra descriptors. + // Some slack for whatever the Go runtime itself opens (GC, netpoll) while + // this runs; the regression this guards produced exactly `candidates` + // extra descriptors, so the bar does not need to be tight. require.Less(t, countFDs()-before, 16, "probing %d candidate directories must not retain a descriptor per directory", candidates) } diff --git a/cmd/entire/cli/repo_mirror.go b/cmd/entire/cli/repo_mirror.go index f891502d53..a496b0d32c 100644 --- a/cmd/entire/cli/repo_mirror.go +++ b/cmd/entire/cli/repo_mirror.go @@ -8,6 +8,7 @@ import ( "io" "net" "net/url" + "os" "regexp" "slices" "strconv" @@ -15,6 +16,7 @@ import ( "time" "charm.land/lipgloss/v2" + "github.com/google/uuid" "github.com/spf13/cobra" "github.com/entireio/cli/internal/coreapi" @@ -493,10 +495,16 @@ func newRepoMirrorCreateCmd() *cobra.Command { " entire repo mirror create github.com/octocat/hello-world aws-us-east-2.entire.io", Args: cobra.RangeArgs(0, 2), RunE: func(cmd *cobra.Command, args []string) error { - opts := mirrorCreateOptions{async: true, noWait: noWait, timeout: waitTimeout} - if settings, err := LoadEntireSettings(cmd.Context()); err == nil { - opts.async = settings.IsAsyncMirrorRequestsEnabled() + opts := mirrorCreateOptions{noWait: noWait, timeout: waitTimeout} + // The route is chosen for us by resolveAsyncMirrorRequests. A + // settings read that failed is reported rather than swallowed — + // on this command it means the cwd has a broken .entire — but it + // does not change the route, which stays on the default. + async, asyncErr := resolveAsyncMirrorRequests(cmd.Context()) + if asyncErr != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "warning: could not read Entire settings (%v); keeping the default mirror-create route\n", asyncErr) } + opts.async = async if len(args) == 0 { return runMirrorCreateWizard(cmd, opts) } @@ -524,12 +532,24 @@ func newRepoMirrorCreateCmd() *cobra.Command { } return runCoreForCluster(cmd, clusterHost, func(ctx context.Context, c *coreapi.Client) error { errW := cmd.ErrOrStderr() + // One spinner for the whole create, relabelled as it moves + // through its phases. Starting a fresh spinner per phase would + // stamp a ✓ completion line on each — three success claims for + // one operation, including on the non-animated path where the + // frames are suppressed but the completion lines are not — and + // would mark a phase successful merely because the next one + // superseded it. + phaseMsg := func(p mirrorCreatePhase) string { + return fmt.Sprintf("%s mirror %s/%s into %s", p.label(), owner, repo, clusterHost) + } + var updatePhase func(string) var finishPhase func(bool) opts.onPhase = func(next mirrorCreatePhase) { - if finishPhase != nil { - finishPhase(true) + if updatePhase == nil { + updatePhase, finishPhase = startUpdatableSpinner(errW, phaseMsg(next)) + return } - finishPhase = startSpinner(errW, fmt.Sprintf("%s mirror %s/%s into %s", next.label(), owner, repo, clusterHost)) + updatePhase(phaseMsg(next)) } outcome, err := createAndAwaitMirror(ctx, c, owner, repo, clusterHost, opts) if finishPhase != nil { @@ -539,16 +559,64 @@ func newRepoMirrorCreateCmd() *cobra.Command { }) }, } - cmd.Flags().BoolVar(&noWait, "no-wait", false, "Return once the placement is registered, without waiting for the initial clone") - cmd.Flags().DurationVar(&waitTimeout, "wait-timeout", 30*time.Minute, "How long to wait. Async mode applies one deadline to request submission, placement, and clone readiness; synchronous mode applies it only to clone readiness") + cmd.Flags().BoolVar(&noWait, "no-wait", false, "Return once the placement is registered, without waiting for the initial clone (on the async route the placement itself is still awaited)") + cmd.Flags().DurationVar(&waitTimeout, "wait-timeout", 30*time.Minute, "How long to wait for mirror creation to finish, covering submission, placement, and the initial clone on both routes") return cmd } +// asyncMirrorRequestsEnv opts into (or out of) the asynchronous +// mirror-request route without touching repo settings. Both the numeric and +// the word spelling are accepted, in both directions. +const ( + asyncMirrorRequestsEnv = "ENTIRE_ASYNC_MIRROR_REQUESTS" + asyncEnvWordOn = "true" + asyncEnvWordOff = "false" +) + +// resolveAsyncMirrorRequests decides whether `repo mirror create` uses the +// asynchronous mirror-request route. Async is the default; the settings key +// and this env var are both opt-outs. +// +// The env var wins over settings, in both directions. `repo mirror create` +// names a repo the caller has usually NOT cloned — it is run from a home +// directory, from an unrelated repo, or before the mirror exists — so a +// switch readable only from the cwd's `.entire/settings.json` has no effect +// exactly where the command is most used. And because that file is +// version-controlled, the key alone lets any repo pin the route for every +// contributor standing in it; the env var is the per-user way back out. +// +// A settings-load error is returned so the caller can report it (on this +// command it means the cwd has a broken `.entire`) but does NOT change the +// route: a read that failed is not evidence the repo opted out, so the +// default stands rather than silently downgrading everyone with a broken +// `.entire` onto the synchronous path. +func resolveAsyncMirrorRequests(ctx context.Context) (bool, error) { + switch os.Getenv(asyncMirrorRequestsEnv) { + case "1", asyncEnvWordOn: + return true, nil + case "0", asyncEnvWordOff: + return false, nil + } + s, err := LoadEntireSettings(ctx) + if err != nil { + return true, err + } + return s.IsAsyncMirrorRequestsEnabled(), nil +} + // mirrorCreateOutcome bundles the create response with the clone status // observed while waiting. polled is false for --no-wait and for empty upstreams, // where there is nothing to await; in those cases status is unset. +// +// requestID is set as soon as an async submission is accepted, and stays set +// even when the wait then fails: it is the caller's only handle on a placement +// that is still progressing server-side, so a timeout must report it rather +// than discard it. It is the zero UUID on the synchronous route, which has no +// such handle (and needs none — CreateMirror is idempotent on +// (upstream, cluster) and returns a mirror id before any polling starts). type mirrorCreateOutcome struct { created *coreapi.CreatedMirror + requestID uuid.UUID status coreapi.MirrorStatus polled bool createdStateUnknown bool @@ -573,6 +641,27 @@ type mirrorCreateOptions struct { onPhase func(mirrorCreatePhase) } +// createAndAwaitMirror is the single create-then-wait path shared by the +// `repo mirror create ` one-shot and the onboarding wizard, so both +// report identical lifecycle states. It registers the GitHub mirror on +// clusterHost (idempotent on (upstream, cluster)) and, unless noWait or the +// upstream is empty, polls the control plane until the clone reaches a terminal +// status. The returned error is the create error (when outcome.created is nil) +// or the wait error — a status sentinel (errMirrorCloneFailed / +// errMirrorSuspended) or a timeout; callers read outcome.status for the state. +// +// opts.onPhase (may be nil) fires as the create moves between phases, so +// callers can render "queued"/"placing"/"cloning" as distinct steps. It is +// called only on a change, and never re-reports the phase it is already in. +// +// opts.async picks the asynchronous mirror-request route. The two routes are +// deliberately indistinguishable to callers by the time they return: the async +// route reaches the same outcome shape, including created.Suspended, which its +// own placement response does not carry. +// +// opts.timeout covers the whole operation on both routes — submission, +// placement, and the initial clone — so the same flag can't mean two things +// depending on which route a setting selected. func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, clusterHost string, opts mirrorCreateOptions) (mirrorCreateOutcome, error) { var currentPhase mirrorCreatePhase reportPhase := func(phase mirrorCreatePhase) { @@ -584,12 +673,13 @@ func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, c } waitCtx := ctx - if opts.async && opts.timeout > 0 { + if opts.timeout > 0 { var cancel context.CancelFunc waitCtx, cancel = context.WithTimeout(ctx, opts.timeout) defer cancel() } + outcome := mirrorCreateOutcome{createdStateUnknown: opts.async} var created *coreapi.CreatedMirror var err error if opts.async { @@ -602,12 +692,14 @@ func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, c }) if submitErr != nil { if waitErr := waitCtx.Err(); waitErr != nil { - return mirrorCreateOutcome{}, classifyWaitContextErr(waitErr, "submitting mirror request") + return outcome, classifyWaitContextErr(waitErr, "submitting mirror request") } - return mirrorCreateOutcome{}, submitErr + return outcome, submitErr } - location, _ := accepted.Location.Get() - created, err = awaitMirrorPlacement(waitCtx, c, accepted.Response, location, func(status coreapi.MirrorRequestStatus) { + // Recorded before the wait, so a placement that times out or exhausts + // its poll-error budget still hands the id back to the caller. + outcome.requestID = accepted.Response.RequestId + created, err = awaitMirrorPlacement(waitCtx, c, accepted.Response, func(status coreapi.MirrorRequestStatus) { switch status { case coreapi.MirrorRequestStatusPending: reportPhase(mirrorCreatePhaseQueued) @@ -618,17 +710,29 @@ func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, c }) } else { reportPhase(mirrorCreatePhasePlacing) - created, err = c.CreateMirror(ctx, &coreapi.CreateMirrorInputBody{ + created, err = c.CreateMirror(waitCtx, &coreapi.CreateMirrorInputBody{ Provider: coreapi.CreateMirrorInputBodyProviderGithub, Owner: owner, Repo: repo, ClusterHost: clusterHost, }) + if err != nil { + // The deadline now covers this call too (it previously wrapped only + // the clone poll, leaving a hanging CreateMirror unbounded), so + // classify it the way the async submission is classified rather + // than surfacing a raw transport error. + if waitErr := waitCtx.Err(); waitErr != nil { + return outcome, classifyWaitContextErr(waitErr, "registering the mirror") + } + } } if err != nil { - return mirrorCreateOutcome{}, err + return outcome, err + } + outcome.created = created + if opts.async { + applyAsyncSuspension(waitCtx, c, created) } - outcome := mirrorCreateOutcome{created: created, createdStateUnknown: opts.async} if created.Suspended { // The placement already existed and an admin has suspended it, so it // will never serve — skip the clone poll. The caller warns after echoing @@ -658,16 +762,42 @@ func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, c return outcome, nil } reportPhase(mirrorCreatePhaseCloning) - pollTimeout := opts.timeout - if opts.async { - pollTimeout = 0 - } - status, werr := awaitMirrorReady(waitCtx, c, created.MirrorId, pollTimeout) + // Timeout is already on waitCtx for both routes, so the poll adds none of + // its own — otherwise --wait-timeout would be spent twice on the sync + // route (once on create, again on the clone). + status, werr := awaitMirrorReady(waitCtx, c, created.MirrorId, 0) outcome.status = status outcome.polled = true return outcome, werr } +// applyAsyncSuspension fills in created.Suspended on the asynchronous route, +// where the placement response cannot carry it. +// +// MirrorRequestResult has mirrorId/mirrorUrl/publicUrl and no `suspended` +// field, while the synchronous CreateMirror response does — and every +// downstream branch (reportOneShotMirror, createOneMirror, the clone-poll +// skip) reads created.Suspended. Without this, an async create against a +// placement an admin suspended reports plain success and exits 0, where the +// sync route warns and exits non-zero; a script chaining `create --no-wait && +// git clone` would then proceed and fail at the clone. One status read closes +// that gap for every caller at once. +// +// Best-effort, matching the empty-upstream suspension probe in +// createAndAwaitMirror: a transient GetMirror error leaves Suspended false +// rather than failing a create that did succeed. The durable fix is +// server-side — adding `suspended` to +// MirrorRequestResult — after which this helper should go. +func applyAsyncSuspension(ctx context.Context, c mirrorStatusGetter, created *coreapi.CreatedMirror) { + m, err := c.GetMirror(ctx, coreapi.GetMirrorParams{MirrorId: created.MirrorId}) + if err != nil { + return + } + if s, ok := m.Status.Get(); ok && s == coreapi.MirrorStatusSuspended { + created.Suspended = true + } +} + // reportOneShotMirror renders the human output for `repo mirror create // ` from the shared createAndAwaitMirror result. A nil // outcome.created means CreateMirror itself failed — surface that error (nothing @@ -675,6 +805,21 @@ func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, c func reportOneShotMirror(out, errW io.Writer, outcome mirrorCreateOutcome, err error) error { created := outcome.created if created == nil { + // No placement to echo. If the request was accepted and the wait then + // ended WITHOUT the server reaching a verdict — a timeout, or an + // exhausted poll budget — name the request: it may well still be + // progressing server-side, and the id is the only thing that + // identifies it, so re-running is otherwise a blind resubmit. + // + // A terminal failure is the opposite case and must not print this: the + // server is done, nothing is in flight, and pairing "repo_inaccessible" + // with "may still be progressing, re-run it" would send the user in + // circles. Such a message already carries its own retry advice when + // retrying is in fact the remedy. + var placementFailed *mirrorPlacementFailedError + if outcome.requestID != uuid.Nil && !errors.As(err, &placementFailed) { + fmt.Fprintf(errW, "\nThe mirror request was accepted (request ID: %s) and may still be progressing.\nRe-run the same command to pick it up — creation is idempotent on (upstream, cluster).\n", outcome.requestID) + } return err } switch { diff --git a/cmd/entire/cli/repo_mirror_create_wizard.go b/cmd/entire/cli/repo_mirror_create_wizard.go index 4e49995b70..8b7151fed0 100644 --- a/cmd/entire/cli/repo_mirror_create_wizard.go +++ b/cmd/entire/cli/repo_mirror_create_wizard.go @@ -572,8 +572,17 @@ func createOneMirror(ctx context.Context, t mirrorTarget, c *coreapi.Client, cli opts.onPhase = func(phase mirrorCreatePhase) { report(string(phase), false, false) } outcome, err := createAndAwaitMirror(ctx, c, t.owner, t.repo, t.region.host, opts) if outcome.created == nil { - res.status, res.err = mirrorStatusError, renderCoreError(err) - report(mirrorStatusError, true, false) + // A deadline can expire before there is any placement to report — on + // the async route the whole placement wait sits here. Classify it as + // timed-out rather than error, so the batch table doesn't render the + // same user-visible condition two ways depending on which side of the + // placement/clone boundary the single --wait-timeout ran out on. + if errors.Is(err, context.DeadlineExceeded) { + res.status, res.err = mirrorStatusTimedOut, err + } else { + res.status, res.err = mirrorStatusError, renderCoreError(err) + } + report(res.status, true, false) return res } res.cloneURL = outcome.created.MirrorUrl diff --git a/cmd/entire/cli/repo_mirror_probe.go b/cmd/entire/cli/repo_mirror_probe.go index 2ff1729059..b8726daad9 100644 --- a/cmd/entire/cli/repo_mirror_probe.go +++ b/cmd/entire/cli/repo_mirror_probe.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "net/url" "regexp" "strings" "time" @@ -91,12 +90,32 @@ func matchGitHubURL(rawURL string, res ...*regexp.Regexp) (owner, repo string, e return "", "", fmt.Errorf("not a recognized GitHub URL: %s", rawURL) } -// mirrorPollInterval is the cadence for placement and clone-status polls. +// mirrorPollInterval is the cadence between placement and clone-status polls. +// A package var (not const) so tests can shorten it. var mirrorPollInterval = 2 * time.Second -// maxConsecutivePollErrors keeps both poll phases bounded. In the clone phase, -// 15 attempts at the two-second cadence preserve the accepted ~30s tolerance -// for a newly placed mirror to become visible. +// maxConsecutivePollErrors bounds how many back-to-back poll failures a wait +// tolerates before giving up. Both phases share the budget, for two different +// reasons. +// +// The clone wait has two failure modes: a brief network/API glitch during a +// long clone, and — the common one — the stale-read window right after create, +// where the control plane returns 404 "mirror not found" because the +// just-written repo#list grant / placement row isn't yet visible to the +// region's minimize_latency + follower reads (~4.8s nominal, but it spikes +// under concurrent multi-region creates). At the 2s cadence, 15 tolerated +// errors ≈ 30s — enough to ride out that window, while a genuinely persistent +// error (deleted mirror, revoked auth) still surfaces well before the 30m +// --wait-timeout. This is a stopgap: the durable fix is server-side, making +// GetMirror check the grant fully-consistent and read the row from the CRDB +// leaseholder so a fresh mirror is visible on the first poll. +// +// The placement wait reuses the number rather than tuning a second one. Its +// failure profile is different — the request row is written before the 202 +// returns, so it has no equivalent stale-read window and only transient +// control-plane errors land here — but ~30s of tolerance is comfortably inside +// the same --wait-timeout. Split the constant rather than re-tuning it if the +// two phases ever need different budgets. const maxConsecutivePollErrors = 15 var ( @@ -118,16 +137,40 @@ type mirrorRequestGetter interface { GetMirrorRequest(ctx context.Context, params coreapi.GetMirrorRequestParams) (*coreapi.MirrorRequest, error) } -func awaitMirrorPlacement(ctx context.Context, c mirrorRequestGetter, initial coreapi.MirrorRequest, location string, onStatus func(coreapi.MirrorRequestStatus)) (*coreapi.CreatedMirror, error) { - serverURL, requestID, err := mirrorRequestPollTarget(location) - if err != nil { - return nil, err - } - if serverURL != nil { - ctx = coreapi.WithServerURL(ctx, serverURL) - } - if initial.RequestId != requestID { - return nil, fmt.Errorf("mirror request Location identifies %s but response identifies %s", requestID, initial.RequestId) +// awaitMirrorPlacement polls a submitted mirror request until it reaches a +// terminal status, returning the placement it produced. It returns: +// +// - the placed mirror when the request succeeded +// - a failure error when the request reached "failed" (see +// mirrorRequestFailureError for how the server's code/message render) +// - a timeout/transport err when the wait deadline passed, or polls kept +// erroring past maxConsecutivePollErrors +// +// The request id comes from the 202 body's required requestId, never from the +// Location header. Two reasons, both load-bearing: +// +// Location is an optional response header in the spec, so requiring it would +// fail a create whose body already reports a succeeded placement — and the +// body is what GetMirrorRequest needs anyway (the id alone addresses it). +// +// More importantly, the header's HOST must never become a poll target. +// Re-pointing the client's base URL at a server-named origin would send the +// control-plane bearer there, which is precisely what the cross-juris +// transport's federation-manifest check exists to prevent. Polling therefore +// stays on the client's configured base URL and lets the cross-jurisdiction +// transport handle a 421 to the home core on every tick (see +// internal/coreapi/cross_juris_client.go, which wires up the shared +// auth-go/crossjuris follower). That also keeps the wait recoverable for its +// whole duration: the home core answers a foreign-region login JWT with a bare +// 401, and the follower treats that as an exchange trigger only on a hop it +// reached by following a 421. Short-circuiting straight to the home core skips +// the redirect, so it works only until the exchanged token leaves the +// transport's cache and then fails unrecoverably — strictly worse than not +// optimising at all. +func awaitMirrorPlacement(ctx context.Context, c mirrorRequestGetter, initial coreapi.MirrorRequest, onStatus func(coreapi.MirrorRequestStatus)) (*coreapi.CreatedMirror, error) { + requestID := initial.RequestId + if requestID == uuid.Nil { + return nil, errors.New("mirror request response is missing a request id") } ticker := time.NewTicker(mirrorPollInterval) @@ -154,6 +197,15 @@ func awaitMirrorPlacement(ctx context.Context, c mirrorRequestGetter, initial co return nil, mirrorRequestFailureError(*request) case coreapi.MirrorRequestStatusPending, coreapi.MirrorRequestStatusProcessing: default: + // Defensive only, and deliberately kept: MirrorRequestStatus is a + // generated CLOSED enum, so a status this CLI doesn't know fails in + // MirrorRequestStatus.UnmarshalText and surfaces as a decode error + // from the GetMirrorRequest below — retried, then reported — rather + // than reaching here. That means a server-side status addition is + // NOT handled the way the operation's contract asks (unknown values + // treated as generic terminal states); honouring it needs the enum + // generated open, which is a spec change. Until then this branch + // only fires if the generator's output changes shape. return nil, fmt.Errorf("mirror request returned unknown status %q", request.Status) } @@ -182,39 +234,27 @@ func awaitMirrorPlacement(ctx context.Context, c mirrorRequestGetter, initial co } } -func mirrorRequestPollTarget(location string) (*url.URL, uuid.UUID, error) { - if strings.TrimSpace(location) == "" { - return nil, uuid.Nil, errors.New("mirror request response is missing Location") - } - locationURL, err := url.Parse(location) - if err != nil || locationURL.User != nil || locationURL.RawQuery != "" || locationURL.Fragment != "" { - return nil, uuid.Nil, fmt.Errorf("invalid mirror request Location %q", location) - } - const prefix = "/api/v1/mirror-requests/" - requestIDText, ok := strings.CutPrefix(locationURL.Path, prefix) - if !ok || requestIDText == "" || strings.Contains(requestIDText, "/") { - return nil, uuid.Nil, fmt.Errorf("invalid mirror request Location %q", location) - } - requestID, err := uuid.Parse(requestIDText) - if err != nil { - return nil, uuid.Nil, fmt.Errorf("invalid mirror request Location %q: %w", location, err) - } - if locationURL.IsAbs() { - if locationURL.Scheme != "https" && locationURL.Scheme != "http" { - return nil, uuid.Nil, fmt.Errorf("invalid mirror request Location %q", location) - } - return &url.URL{Scheme: locationURL.Scheme, Host: locationURL.Host, Path: "/api/v1"}, requestID, nil - } - if locationURL.Host != "" { - return nil, uuid.Nil, fmt.Errorf("invalid mirror request Location %q", location) - } - return nil, requestID, nil -} +// mirrorPlacementFailedError reports a mirror request that reached the +// terminal "failed" status. It is distinguishable from a timeout or an +// exhausted poll budget because nothing is left in flight: the server is done +// with this request, so callers must not invite the user to wait for it or +// describe it as still progressing. Retry advice, where retrying is the +// remedy, is already part of the message. +type mirrorPlacementFailedError struct{ msg string } + +func (e *mirrorPlacementFailedError) Error() string { return e.msg } +// mirrorRequestFailureError renders a terminal "failed" mirror request as a +// user-facing error. The failure code is a free-form string in the contract, +// which explicitly asks callers to treat an unknown code as a generic terminal +// failure — so an unrecognised code still produces a usable message (quoting +// the code and the server's own text) rather than being dropped. Retryable +// failures say so, since the command is idempotent on (upstream, cluster) and +// re-running it is the whole remedy. func mirrorRequestFailureError(request coreapi.MirrorRequest) error { failure, ok := request.Failure.Get() if !ok { - return errors.New("mirror placement failed without failure details") + return &mirrorPlacementFailedError{msg: "mirror placement failed without failure details"} } known := false @@ -233,7 +273,7 @@ func mirrorRequestFailureError(request coreapi.MirrorRequest) error { if failure.Retryable { message += "; retry this command" } - return errors.New(message) + return &mirrorPlacementFailedError{msg: message} } // awaitMirrorReady polls the control plane for a mirror's clone lifecycle until diff --git a/cmd/entire/cli/repo_mirror_request_test.go b/cmd/entire/cli/repo_mirror_request_test.go index 3d4fa8bc5e..c48abf031d 100644 --- a/cmd/entire/cli/repo_mirror_request_test.go +++ b/cmd/entire/cli/repo_mirror_request_test.go @@ -88,6 +88,8 @@ func TestCreateAndAwaitMirror_AsyncSuccess(t *testing.T) { writeAcceptedMirrorRequest(t, w) case mirrorRequestPath(): writeSuccessfulMirrorRequest(t, w) + case mirrorStatusAPIPath: + writeMirrorStatus(t, w, coreapi.MirrorStatusReady) default: t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) } @@ -99,7 +101,11 @@ func TestCreateAndAwaitMirror_AsyncSuccess(t *testing.T) { require.NoError(t, err) require.Equal(t, "mirror-1", outcome.created.MirrorId) require.False(t, outcome.polled) - require.Equal(t, []string{mirrorRequestsAPIPath, mirrorRequestPath()}, paths) + // --no-wait still reads the mirror's status once: the placement + // response cannot say whether an existing placement is suspended, and + // reporting a suspended mirror as a plain success is what sends a + // script on to a clone that then fails. + require.Equal(t, []string{mirrorRequestsAPIPath, mirrorRequestPath(), mirrorStatusAPIPath}, paths) }) } @@ -202,6 +208,8 @@ func TestCreateAndAwaitMirror_AsyncFailures(t *testing.T) { return } writeSuccessfulMirrorRequest(t, w) + case mirrorStatusAPIPath: + writeMirrorStatus(t, w, coreapi.MirrorStatusReady) default: t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) } @@ -233,26 +241,74 @@ func TestCreateAndAwaitMirror_AsyncFailures(t *testing.T) { }) } -func TestCreateAndAwaitMirror_AsyncLocationValidation(t *testing.T) { +// TestCreateAndAwaitMirror_AsyncIgnoresLocation pins that the poll is driven by +// the 202 body's requestId, not the Location header. Location is optional in +// the spec, so a create whose body already identifies the request must not fail +// on a missing or unparseable header — and the header's host must never become +// a poll target, since re-pointing the client's base URL at a server-named +// origin would send the control-plane bearer there. +func TestCreateAndAwaitMirror_AsyncIgnoresLocation(t *testing.T) { useFastMirrorPolling(t) - for _, location := range []string{"", ":", "/api/v1/mirrors/not-a-request", "/api/v1/mirror-requests/not-a-uuid", mirrorRequestPath() + "?extra=true"} { - t.Run("invalid Location "+location, func(t *testing.T) { - client := newMirrorRequestClient(t, func(w http.ResponseWriter, _ *http.Request) { - if location != "" { - w.Header().Set("Location", location) + locations := []string{ + "", + ":", + "/api/v1/mirrors/not-a-request", + "/api/v1/mirror-requests/not-a-uuid", + mirrorRequestPath() + "?extra=true", + "http://evil.example/api/v1/mirror-requests/" + testMirrorRequestID.String(), + "/some-prefix/api/v1/mirror-requests/" + testMirrorRequestID.String(), + } + for _, location := range locations { + t.Run("Location "+location, func(t *testing.T) { + var hosts []string + client := newMirrorRequestClient(t, func(w http.ResponseWriter, r *http.Request) { + hosts = append(hosts, r.Host) + switch r.URL.Path { + case mirrorRequestsAPIPath: + if location != "" { + w.Header().Set("Location", location) + } + writeJSONResponse(t, w, http.StatusAccepted, &coreapi.MirrorRequest{RequestId: testMirrorRequestID, Status: coreapi.MirrorRequestStatusPending}) + case mirrorRequestPath(): + writeSuccessfulMirrorRequest(t, w) + case mirrorStatusAPIPath: + writeMirrorStatus(t, w, coreapi.MirrorStatusReady) + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) } - writeJSONResponse(t, w, http.StatusAccepted, &coreapi.MirrorRequest{RequestId: testMirrorRequestID, Status: coreapi.MirrorRequestStatusPending}) }) - _, err := createAndAwaitMirror(t.Context(), client, "owner", "repo", "cluster", mirrorCreateOptions{ - async: true, timeout: time.Second, + outcome, err := createAndAwaitMirror(t.Context(), client, "owner", "repo", "cluster", mirrorCreateOptions{ + async: true, noWait: true, timeout: time.Second, }) - require.ErrorContains(t, err, "Location") + require.NoError(t, err) + require.Equal(t, "mirror-1", outcome.created.MirrorId) + // Every request stayed on the client's own base URL — nothing was + // re-targeted at the host the Location named. + require.NotEmpty(t, hosts) + for _, host := range hosts { + require.NotEqual(t, "evil.example", host) + } }) } } +// TestCreateAndAwaitMirror_AsyncMissingRequestID pins the one thing the body +// genuinely must carry: without a request id there is nothing to poll. +func TestCreateAndAwaitMirror_AsyncMissingRequestID(t *testing.T) { + useFastMirrorPolling(t) + + client := newMirrorRequestClient(t, func(w http.ResponseWriter, _ *http.Request) { + writeJSONResponse(t, w, http.StatusAccepted, &coreapi.MirrorRequest{Status: coreapi.MirrorRequestStatusPending}) + }) + + _, err := createAndAwaitMirror(t.Context(), client, "owner", "repo", "cluster", mirrorCreateOptions{ + async: true, timeout: time.Second, + }) + require.ErrorContains(t, err, "missing a request id") +} + func TestCreateAndAwaitMirror_AsyncTimeout(t *testing.T) { useFastMirrorPolling(t) @@ -328,17 +384,22 @@ func TestCreateAndAwaitMirror_AsyncCrossJurisdiction(t *testing.T) { case mirrorRequestPath(): homeAuths = append(homeAuths, r.Header.Get("Authorization")) writeSuccessfulMirrorRequest(t, w) + case mirrorStatusAPIPath: + homeAuths = append(homeAuths, r.Header.Get("Authorization")) + writeMirrorStatus(t, w, coreapi.MirrorStatusReady) default: t.Errorf("unexpected home-core request %s %s", r.Method, r.URL.Path) } })) t.Cleanup(homeCore.Close) + var redirected []string wrongCore := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/.well-known/entire-federation" { writeJSONResponse(t, w, http.StatusOK, map[string]any{"peer_auth_hosts": []string{homeCore.URL}}) return } + redirected = append(redirected, r.URL.Path) w.WriteHeader(http.StatusMisdirectedRequest) if _, err := fmt.Fprintf(w, `{"home_core_url":%q}`, homeCore.URL); err != nil { t.Errorf("write 421 response: %v", err) @@ -353,7 +414,15 @@ func TestCreateAndAwaitMirror_AsyncCrossJurisdiction(t *testing.T) { }) require.NoError(t, err) require.Equal(t, "mirror-1", outcome.created.MirrorId) - require.Equal(t, []string{"Bearer original-token", "Bearer home-token", "Bearer home-token"}, homeAuths) + require.Equal(t, []string{"Bearer original-token", "Bearer home-token", "Bearer home-token", "Bearer home-token"}, homeAuths) + // Every call — submission, placement poll, status read — went out to + // the client's configured core and was redirected there. This is the + // regression guard: short-circuiting the poll straight at the home + // core (via WithServerURL from the Location header) strips the + // afterRedirect provenance the transport's bare-401 re-exchange needs, + // so the wait breaks unrecoverably once the exchanged token leaves the + // cache. Polls must keep arriving here. + require.Equal(t, []string{mirrorRequestsAPIPath, mirrorRequestPath(), mirrorStatusAPIPath}, redirected) }) } @@ -374,6 +443,8 @@ func TestCreateAndAwaitMirror_AsyncResubmission(t *testing.T) { writeAcceptedMirrorRequest(t, w) case mirrorRequestPath(): writeSuccessfulMirrorRequest(t, w) + case mirrorStatusAPIPath: + writeMirrorStatus(t, w, coreapi.MirrorStatusReady) default: t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) } @@ -456,6 +527,8 @@ func TestRepoMirrorCreate_AsyncDefaultWhenSettingsFail(t *testing.T) { return } writeSuccessfulMirrorRequest(t, w) + case mirrorStatusAPIPath: + writeMirrorStatus(t, w, coreapi.MirrorStatusReady) default: t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) } @@ -474,9 +547,12 @@ func TestRepoMirrorCreate_AsyncDefaultWhenSettingsFail(t *testing.T) { require.Contains(t, stdout.String(), "Mirror ID: mirror-1") require.NotContains(t, stdout.String(), "Registered mirror") require.NotContains(t, stdout.String(), "Mirror exists") - require.Contains(t, stderr.String(), "Queued mirror owner/repo") - require.Contains(t, stderr.String(), "Placing mirror owner/repo") - require.Equal(t, []string{mirrorRequestsAPIPath, mirrorRequestPath(), mirrorRequestPath()}, paths) + // One spinner for the whole create, so exactly one completion line — three + // ✓ lines would be three success claims for one operation, and would mark + // a phase successful merely because the next one superseded it. + require.Equal(t, 1, strings.Count(stderr.String(), "✓"), "stderr: %q", stderr.String()) + require.Contains(t, stderr.String(), "mirror owner/repo into aws-us-east-2.entire.io") + require.Equal(t, []string{mirrorRequestsAPIPath, mirrorRequestPath(), mirrorRequestPath(), mirrorStatusAPIPath}, paths) } func TestRepoMirrorCreate_SynchronousOptOut(t *testing.T) { @@ -551,6 +627,12 @@ func TestCreateMirrors_AsyncKeepsConcurrencyAndFailuresIndependent(t *testing.T) reachedLimit := make(chan struct{}) release := make(chan struct{}) client := newMirrorRequestClient(t, func(w http.ResponseWriter, r *http.Request) { + // The status read applyAsyncSuspension makes carries no body, so route + // it out before decoding one. + if r.URL.Path == mirrorStatusAPIPath { + writeMirrorStatus(t, w, coreapi.MirrorStatusReady) + return + } var body struct { Repo string `json:"repo"` } @@ -648,6 +730,16 @@ func writeAcceptedMirrorRequest(t *testing.T, w http.ResponseWriter) { writeJSONResponse(t, w, http.StatusAccepted, &coreapi.MirrorRequest{RequestId: testMirrorRequestID, Status: coreapi.MirrorRequestStatusPending}) } +// mirrorStatusAPIPath is the status route every async create now reads once, +// so applyAsyncSuspension can fill in CreatedMirror.Suspended (the placement +// response carries no such field). +const mirrorStatusAPIPath = "/api/v1/mirrors/mirror-1" + +func writeMirrorStatus(t *testing.T, w http.ResponseWriter, status coreapi.MirrorStatus) { + t.Helper() + writeJSONResponse(t, w, http.StatusOK, &coreapi.Mirror{Status: coreapi.NewOptMirrorStatus(status)}) +} + func writeSuccessfulMirrorRequest(t *testing.T, w http.ResponseWriter) { t.Helper() writeSuccessfulMirrorRequestWithStatus(t, w, http.StatusOK) @@ -679,3 +771,214 @@ func writeCoreProblem(t *testing.T, w http.ResponseWriter, status int, detail st t.Errorf("write problem response: %v", err) } } + +// TestCreateAndAwaitMirror_AsyncSuspendedPlacement pins that the async route +// reports a suspended placement the same way the synchronous one does. +// MirrorRequestResult has no `suspended` field, so without a status read the +// async create returns Suspended=false and every downstream branch treats an +// unusable mirror as a success — exiting 0 from `create --no-wait`, which sends +// a script chaining `&& git clone` on to a clone that then fails. +func TestCreateAndAwaitMirror_AsyncSuspendedPlacement(t *testing.T) { + useFastMirrorPolling(t) + + newSuspendedClient := func(t *testing.T) *coreapi.Client { + return newMirrorRequestClient(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case mirrorRequestsAPIPath: + writeAcceptedMirrorRequest(t, w) + case mirrorRequestPath(): + writeSuccessfulMirrorRequest(t, w) + case mirrorStatusAPIPath: + writeMirrorStatus(t, w, coreapi.MirrorStatusSuspended) + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + } + }) + } + + t.Run("no-wait surfaces the suspension and exits non-zero", func(t *testing.T) { + outcome, err := createAndAwaitMirror(t.Context(), newSuspendedClient(t), "owner", "repo", "cluster", mirrorCreateOptions{ + async: true, noWait: true, timeout: time.Second, + }) + require.NoError(t, err, "a suspended re-create is a non-fatal create") + require.True(t, outcome.created.Suspended) + + var stdout, stderr bytes.Buffer + reportErr := reportOneShotMirror(&stdout, &stderr, outcome, err) + require.ErrorIs(t, reportErr, errMirrorSuspended) + require.Contains(t, stderr.String(), "suspended by an admin") + require.NotContains(t, stdout.String(), "will work once it completes") + }) + + t.Run("the wizard classifies it as suspended", func(t *testing.T) { + target := mirrorTarget{owner: "owner", repo: "repo", region: regionChoice{host: "cluster"}} + result := createOneMirror(t.Context(), target, newSuspendedClient(t), nil, + mirrorCreateOptions{async: true, noWait: true, timeout: time.Second}, nil) + require.Equal(t, mirrorStatusSuspended, result.status) + require.Error(t, result.err) + }) +} + +// TestCreateAndAwaitMirror_AsyncTimeoutKeepsRequestID pins that a placement +// that outlives --wait-timeout still hands back the request id. It is the only +// handle on a placement that may still be progressing server-side, and there is +// no `mirror request get` subcommand to look one up after the fact, so dropping +// it leaves the user with nothing but a blind resubmit. +func TestCreateAndAwaitMirror_AsyncTimeoutKeepsRequestID(t *testing.T) { + useFastMirrorPolling(t) + + client := newMirrorRequestClient(t, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case mirrorRequestsAPIPath: + writeAcceptedMirrorRequest(t, w) + case mirrorRequestPath(): + writeJSONResponse(t, w, http.StatusOK, &coreapi.MirrorRequest{RequestId: testMirrorRequestID, Status: coreapi.MirrorRequestStatusProcessing}) + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + } + }) + + outcome, err := createAndAwaitMirror(t.Context(), client, "owner", "repo", "cluster", mirrorCreateOptions{ + async: true, timeout: 20 * time.Millisecond, + }) + require.ErrorContains(t, err, "timed out waiting for mirror placement") + require.Nil(t, outcome.created) + require.Equal(t, testMirrorRequestID, outcome.requestID) + + var stdout, stderr bytes.Buffer + require.Error(t, reportOneShotMirror(&stdout, &stderr, outcome, err)) + require.Contains(t, stderr.String(), testMirrorRequestID.String()) + require.Contains(t, stderr.String(), "idempotent") +} + +// TestCreateOneMirror_AsyncPlacementTimeoutIsTimedOut pins that the wizard +// renders a placement timeout as "timed out", not "error". Both halves of the +// single --wait-timeout describe the same user-visible condition, so which side +// of the placement/clone boundary it expired on must not change the label. +func TestCreateOneMirror_AsyncPlacementTimeoutIsTimedOut(t *testing.T) { + useFastMirrorPolling(t) + + client := newMirrorRequestClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == mirrorRequestsAPIPath { + writeAcceptedMirrorRequest(t, w) + return + } + writeJSONResponse(t, w, http.StatusOK, &coreapi.MirrorRequest{RequestId: testMirrorRequestID, Status: coreapi.MirrorRequestStatusProcessing}) + }) + + target := mirrorTarget{owner: "owner", repo: "repo", region: regionChoice{host: "cluster"}} + result := createOneMirror(t.Context(), target, client, nil, + mirrorCreateOptions{async: true, timeout: 20 * time.Millisecond}, nil) + require.Equal(t, mirrorStatusTimedOut, result.status) + require.ErrorIs(t, result.err, context.DeadlineExceeded) +} + +// TestCreateAndAwaitMirror_SyncTimeoutCoversCreate pins that --wait-timeout +// bounds the synchronous route's create call too. It previously wrapped only +// the clone poll, so a CreateMirror that hung ignored the flag entirely — and +// the flag's help now promises it covers mirror creation. +func TestCreateAndAwaitMirror_SyncTimeoutCoversCreate(t *testing.T) { + client := newMirrorRequestClient(t, func(w http.ResponseWriter, _ *http.Request) { + time.Sleep(200 * time.Millisecond) + w.WriteHeader(http.StatusOK) + }) + + start := time.Now() + _, err := createAndAwaitMirror(t.Context(), client, "owner", "repo", "cluster", mirrorCreateOptions{ + timeout: 20 * time.Millisecond, + }) + require.ErrorContains(t, err, "timed out registering the mirror") + require.Less(t, time.Since(start), 150*time.Millisecond) +} + +// TestResolveAsyncMirrorRequests pins the env var's precedence over repo +// settings, in both directions. `repo mirror create` names a repo the caller +// has usually not cloned, so a switch readable only from the cwd's +// .entire/settings.json has no effect where the command is most used; and +// because that file is version-controlled, a repo must not be able to pin the +// route for everyone standing in it with no way out. +func TestResolveAsyncMirrorRequests(t *testing.T) { + for _, tt := range []struct { + name string + settings string + env string + want bool + }{ + {name: "unset and unspecified is the default", settings: `{}`, want: true}, + {name: "unset honours an explicit opt-in", settings: `{"async_mirror_requests":true}`, want: true}, + {name: "unset honours an explicit opt-out", settings: `{"async_mirror_requests":false}`}, + {name: "env enables over an opt-out", settings: `{"async_mirror_requests":false}`, env: "1", want: true}, + {name: "env true enables over an opt-out", settings: `{"async_mirror_requests":false}`, env: asyncEnvWordOn, want: true}, + {name: "env disables over the default", settings: `{}`, env: "0"}, + {name: "env false disables over an explicit opt-in", settings: `{"async_mirror_requests":true}`, env: asyncEnvWordOff}, + } { + t.Run(tt.name, func(t *testing.T) { + setupTestRepo(t) + writeSettings(t, tt.settings) + t.Setenv(asyncMirrorRequestsEnv, tt.env) + + got, err := resolveAsyncMirrorRequests(t.Context()) + require.NoError(t, err) + require.Equal(t, tt.want, got) + }) + } + + // The command is routinely run from outside any repository, so that must + // not become a warning on every invocation. + t.Run("no repository and no env is a clean default", func(t *testing.T) { + t.Chdir(t.TempDir()) + t.Setenv(asyncMirrorRequestsEnv, "") + got, err := resolveAsyncMirrorRequests(t.Context()) + require.NoError(t, err) + require.True(t, got) + }) + + // A settings read that failed says nothing about whether the repo opted + // out, so the default must stand — the caller reports the error, but + // nobody gets silently downgraded onto the synchronous route by a broken + // .entire. + t.Run("a settings error keeps the default route", func(t *testing.T) { + setupTestRepo(t) + writeSettings(t, `{"async_mirror_requests":`) + t.Setenv(asyncMirrorRequestsEnv, "") + got, err := resolveAsyncMirrorRequests(t.Context()) + require.Error(t, err) + require.True(t, got) + }) + + t.Run("env wins with no repository at all", func(t *testing.T) { + t.Chdir(t.TempDir()) + t.Setenv(asyncMirrorRequestsEnv, "1") + got, err := resolveAsyncMirrorRequests(t.Context()) + require.NoError(t, err) + require.True(t, got, "the switch must work outside a repo, which is where this command is normally run") + }) +} + +// TestReportOneShotMirror_TerminalFailureIsNotCalledInFlight pins that the +// "may still be progressing" notice is scoped to waits that ended without a +// verdict. A terminal placement failure carries a request id too, and pairing +// e.g. repo_inaccessible with "re-run it to pick it up" sends the user in +// circles over work the server has already finished with. +func TestReportOneShotMirror_TerminalFailureIsNotCalledInFlight(t *testing.T) { + t.Parallel() + + failed := coreapi.MirrorRequest{RequestId: testMirrorRequestID, Status: coreapi.MirrorRequestStatusFailed} + failed.Failure = coreapi.NewOptMirrorRequestFailure(coreapi.MirrorRequestFailure{ + Code: "repo_inaccessible", Message: "the upstream is not reachable", + }) + outcome := mirrorCreateOutcome{requestID: testMirrorRequestID, createdStateUnknown: true} + + var stdout, stderr bytes.Buffer + err := reportOneShotMirror(&stdout, &stderr, outcome, mirrorRequestFailureError(failed)) + require.ErrorContains(t, err, "repo_inaccessible") + require.NotContains(t, stderr.String(), "may still be progressing") + require.NotContains(t, stderr.String(), "Re-run") + + // A wait that ended with no verdict still names the request. + var timedOutOut, timedOutErr bytes.Buffer + require.Error(t, reportOneShotMirror(&timedOutOut, &timedOutErr, outcome, + classifyWaitContextErr(context.DeadlineExceeded, "waiting for mirror placement"))) + require.Contains(t, timedOutErr.String(), testMirrorRequestID.String()) + require.Contains(t, timedOutErr.String(), "may still be progressing") +} diff --git a/cmd/entire/cli/repo_mirror_test.go b/cmd/entire/cli/repo_mirror_test.go index 4aa1125479..f240847537 100644 --- a/cmd/entire/cli/repo_mirror_test.go +++ b/cmd/entire/cli/repo_mirror_test.go @@ -227,10 +227,15 @@ func TestCreateAndAwaitMirror_SynchronousPhases(t *testing.T) { func TestRepoMirrorCreate_WaitTimeoutHelp(t *testing.T) { t.Parallel() + // The two routes used to spend this flag differently — async wrapped the + // whole operation, sync bounded only the clone poll, leaving a hanging + // CreateMirror unbounded — and the help text described that split. The + // deadline now covers submission, placement, and clone on both, so the + // help promises one thing and the test pins that it still does. flag := newRepoMirrorCreateCmd().Flags().Lookup("wait-timeout") require.NotNil(t, flag) - require.Contains(t, flag.Usage, "Async mode applies one deadline to request submission, placement, and clone readiness") - require.Contains(t, flag.Usage, "synchronous mode applies it only to clone readiness") + require.Contains(t, flag.Usage, "covering submission, placement, and the initial clone on both routes") + require.NotContains(t, flag.Usage, "only to clone readiness") } // TestReportOneShotMirror exercises the one-shot create's presentation across diff --git a/internal/coreapi/cross_juris_client.go b/internal/coreapi/cross_juris_client.go index e283b0ad11..018a650b8a 100644 --- a/internal/coreapi/cross_juris_client.go +++ b/internal/coreapi/cross_juris_client.go @@ -36,9 +36,17 @@ func newCrossJurisHTTPClient(coreURL string) (*http.Client, error) { return &http.Client{Transport: rt}, nil } -// newCrossJurisRoundTripper stacks the coreapi transport chain over base: -// the shared crossjuris follower, then the CLI-specific Location -// canonicalizer on the outside so it sees the replayed request. +// newCrossJurisRoundTripper builds the coreapi transport over base: the +// shared crossjuris follower, and nothing else. +// +// It used to stack a CLI-specific canonicalizer on the outside, rewriting the +// Location a 202 from POST /api/v1/mirror-requests carries onto the origin +// that answered. Nothing reads that header any more — the async mirror-create +// poll is driven by the 202 body's requestId and stays on the client's own +// base URL (see awaitMirrorPlacement) — so the rewrite was maintaining a +// header no caller consumed, over a value the server supplies. Reviving it +// means giving a server-named host a say in where the control-plane bearer is +// sent, which is what the follower's federation check exists to gate. func newCrossJurisRoundTripper(base http.RoundTripper, allowInsecureHTTP bool) (http.RoundTripper, error) { inner, err := crossjuris.New(crossjuris.Config{ Base: base, @@ -49,7 +57,7 @@ func newCrossJurisRoundTripper(base http.RoundTripper, allowInsecureHTTP bool) ( if err != nil { return nil, fmt.Errorf("cross-juris transport: %w", err) } - return mirrorLocationCanonicalizer{next: inner}, nil + return inner, nil } // debugf writes ENTIRE_DEBUG-gated trace lines for the transport. The @@ -63,42 +71,6 @@ func debugf(format string, args ...any) { fmt.Fprintf(os.Stderr, "[entire] cross-juris transport: "+format+"\n", args...) } -// mirrorLocationCanonicalizer rewrites the Location a 202 from -// POST /api/v1/mirror-requests carries onto the origin that actually -// answered. The home core emits a relative or self-rooted Location; after -// a 421 follow that is a different host than the caller dialled, so the -// scheme and host come from resp.Request — the replayed request — not -// from the caller's. -type mirrorLocationCanonicalizer struct { - next http.RoundTripper -} - -func (c mirrorLocationCanonicalizer) RoundTrip(req *http.Request) (*http.Response, error) { - resp, err := c.next.RoundTrip(req) - if err != nil { - return nil, err //nolint:wrapcheck // http.Client already names method and URL - } - canonicalizeMirrorRequestLocation(req, resp) - return resp, nil -} - -func canonicalizeMirrorRequestLocation(req *http.Request, resp *http.Response) { - answered := resp.Request - if answered == nil { - answered = req - } - if resp.StatusCode != http.StatusAccepted || answered.URL.Path != apiBasePath+"/mirror-requests" { - return - } - location, err := url.Parse(resp.Header.Get("Location")) - if err != nil || location.Path == "" { - return - } - location.Scheme = answered.URL.Scheme - location.Host = answered.URL.Host - resp.Header.Set("Location", location.String()) -} - // isLoopbackHTTP reports whether rawURL is http:// at a loopback host. func isLoopbackHTTP(rawURL string) bool { u, err := url.Parse(rawURL) diff --git a/internal/coreapi/cross_juris_client_test.go b/internal/coreapi/cross_juris_client_test.go index a2148dec2e..de0396b9e5 100644 --- a/internal/coreapi/cross_juris_client_test.go +++ b/internal/coreapi/cross_juris_client_test.go @@ -185,38 +185,6 @@ func TestRoundTripper_421FollowsToHomeCore(t *testing.T) { } } -// TestRoundTripper_421CanonicalizesMirrorRequestLocation: the Location -// on a 202 from mirror-requests names the core that answered (the home -// core after a follow), not the one the caller dialled. -func TestRoundTripper_421CanonicalizesMirrorRequestLocation(t *testing.T) { - t.Parallel() - homeCore := newCrossJurisTestServer(t, func(s *crossJurisTestServer, w http.ResponseWriter, r *http.Request) { - s.record(r) - w.Header().Set("Location", "/api/v1/mirror-requests/67b477f3-97b7-4dfe-90c4-6365dbebd5bf") - w.WriteHeader(http.StatusAccepted) - }) - wrongCore := newCrossJurisTestServer(t, func(s *crossJurisTestServer, w http.ResponseWriter, r *http.Request) { - s.record(r) - w.WriteHeader(http.StatusMisdirectedRequest) - w.Write([]byte(`{"home_core_url":"` + homeCore.srv.URL + `"}`)) //nolint:errcheck // test - }) - wrongCore.peers = []string{homeCore.srv.URL} - - client := &http.Client{Transport: transportFor(t)} - req, _ := http.NewRequestWithContext(t.Context(), http.MethodPost, wrongCore.srv.URL+"/api/v1/mirror-requests", strings.NewReader(`{}`)) //nolint:errcheck // test - req.Header.Set("Authorization", "Bearer user-jwt") - - resp, err := client.Do(req) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - want := homeCore.srv.URL + "/api/v1/mirror-requests/67b477f3-97b7-4dfe-90c4-6365dbebd5bf" - if got := resp.Header.Get("Location"); got != want { - t.Fatalf("Location = %q, want %q", got, want) - } -} - // TestRoundTripper_421ThenBareUnauthorizedProactiveExchange is the // production path: after following a 421, the home core can't verify the // foreign-region login JWT's signature and returns a BARE 401 (no hint). From b4d670e36d47585b9ef49e42f040f309965823fa Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Mon, 7 Sep 2026 08:43:49 +0200 Subject: [PATCH 2/2] Bound the empty-upstream suspension read by --wait-timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trail finding 01M1VQ779H5B: the probe read the raw ctx, so it could hang past a deadline the caller had set — and past the doc comment this branch added, which promises opts.timeout covers the whole operation on both routes. Every other call in createAndAwaitMirror already uses waitCtx. The branch is only reachable through the deprecated `empty` flag the server no longer sets, so the test hand-builds that response; it hangs for 5s against the old code and returns in 50ms against the fixed one. Also reflows the applyAsyncSuspension comment paragraph whose ragged wrap Copilot flagged. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01M1X9QPE408H8JK41GBT0K07P --- cmd/entire/cli/repo_mirror.go | 6 +-- cmd/entire/cli/repo_mirror_request_test.go | 46 ++++++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/repo_mirror.go b/cmd/entire/cli/repo_mirror.go index a496b0d32c..46d8b7eeab 100644 --- a/cmd/entire/cli/repo_mirror.go +++ b/cmd/entire/cli/repo_mirror.go @@ -748,7 +748,7 @@ func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, c // finishMirrorCreate behavior; the read is best-effort, so a transient // GetMirror error just falls through to the benign "nothing to clone". if !created.Created { - if m, gerr := c.GetMirror(ctx, coreapi.GetMirrorParams{MirrorId: created.MirrorId}); gerr == nil { + if m, gerr := c.GetMirror(waitCtx, coreapi.GetMirrorParams{MirrorId: created.MirrorId}); gerr == nil { if s, ok := m.Status.Get(); ok && s == coreapi.MirrorStatusSuspended { outcome.status = s outcome.polled = true @@ -786,8 +786,8 @@ func createAndAwaitMirror(ctx context.Context, c *coreapi.Client, owner, repo, c // Best-effort, matching the empty-upstream suspension probe in // createAndAwaitMirror: a transient GetMirror error leaves Suspended false // rather than failing a create that did succeed. The durable fix is -// server-side — adding `suspended` to -// MirrorRequestResult — after which this helper should go. +// server-side — adding `suspended` to MirrorRequestResult — after which this +// helper should go. func applyAsyncSuspension(ctx context.Context, c mirrorStatusGetter, created *coreapi.CreatedMirror) { m, err := c.GetMirror(ctx, coreapi.GetMirrorParams{MirrorId: created.MirrorId}) if err != nil { diff --git a/cmd/entire/cli/repo_mirror_request_test.go b/cmd/entire/cli/repo_mirror_request_test.go index c48abf031d..6741b8903b 100644 --- a/cmd/entire/cli/repo_mirror_request_test.go +++ b/cmd/entire/cli/repo_mirror_request_test.go @@ -982,3 +982,49 @@ func TestReportOneShotMirror_TerminalFailureIsNotCalledInFlight(t *testing.T) { require.Contains(t, timedOutErr.String(), testMirrorRequestID.String()) require.Contains(t, timedOutErr.String(), "may still be progressing") } + +// TestCreateAndAwaitMirror_EmptyUpstreamSuspensionReadIsBounded pins that the +// empty-upstream suspension probe runs under --wait-timeout like every other +// call in createAndAwaitMirror. It read the raw ctx, so it could hang past a +// deadline the caller had set and the doc comment promised covered the whole +// operation. The branch is reached only via the deprecated `empty` flag, which +// the server no longer sets — hence the hand-built response. +func TestCreateAndAwaitMirror_EmptyUpstreamSuspensionReadIsBounded(t *testing.T) { + blocked := make(chan struct{}) + client := newMirrorRequestClient(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/mirrors": + writeJSONResponse(t, w, http.StatusCreated, &coreapi.CreatedMirror{ + Created: false, Empty: true, MirrorId: "mirror-1", MirrorUrl: "entire://cluster/gh/owner/repo", + }) + case r.URL.Path == mirrorStatusAPIPath: + close(blocked) + <-r.Context().Done() + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + } + }) + + type result struct { + outcome mirrorCreateOutcome + err error + } + done := make(chan result, 1) + go func() { + outcome, err := createAndAwaitMirror(t.Context(), client, "owner", "repo", "cluster", mirrorCreateOptions{ + timeout: 50 * time.Millisecond, + }) + done <- result{outcome, err} + }() + + <-blocked + select { + case got := <-done: + // The probe is best-effort: a read cut short by the deadline falls + // through to the benign "nothing to clone", it does not fail the create. + require.NoError(t, got.err) + require.NotNil(t, got.outcome.created) + case <-time.After(5 * time.Second): + t.Fatal("empty-upstream suspension read ignored --wait-timeout and hung") + } +}