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..46d8b7eeab 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 @@ -644,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 @@ -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..6741b8903b 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,260 @@ 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") +} + +// 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") + } +} 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).