Skip to content

Fix async mirror-create review findings from #2207 #2242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Soph wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix async mirror-create review findings from #2207 #2242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a20a65d
Fix async mirror-create review findings from #2207
Soph Sep 2, 2026
b4d670e
Bound the empty-upstream suspension read by --wait-timeout
Soph Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions cmd/entire/cli/agent/session_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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)
}
189 changes: 167 additions & 22 deletions cmd/entire/cli/repo_mirror.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ import (
"io"
"net"
"net/url"
"os"
"regexp"
"slices"
"strconv"
"strings"
"time"

"charm.land/lipgloss/v2"
"github.com/google/uuid"
"github.com/spf13/cobra"

"github.com/entireio/cli/internal/coreapi"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -573,6 +641,27 @@ type mirrorCreateOptions struct {
onPhase func(mirrorCreatePhase)
}

// createAndAwaitMirror is the single create-then-wait path shared by the
// `repo mirror create <github-url>` 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) {
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -658,23 +762,64 @@ 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
// <github-url>` from the shared createAndAwaitMirror result. A nil
// outcome.created means CreateMirror itself failed — surface that error (nothing
// was printed yet). Otherwise echo the placement, then the lifecycle outcome.
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)
}
Comment thread
Soph marked this conversation as resolved.
return err
}
switch {
Expand Down
13 changes: 11 additions & 2 deletions cmd/entire/cli/repo_mirror_create_wizard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading