diff --git a/cmd/entire/cli/checkpoint/open.go b/cmd/entire/cli/checkpoint/open.go index 13e954b922..391e4d1b8b 100644 --- a/cmd/entire/cli/checkpoint/open.go +++ b/cmd/entire/cli/checkpoint/open.go @@ -167,7 +167,11 @@ func buildMirrors(ctx context.Context, env OpenEnv, cfg *settings.CheckpointsCon return nil, fmt.Errorf("checkpoints.mirrors[%d]: backend type %q is already used by the primary or another mirror; each backend type may appear at most once", i, m.Type) } seen[m.Type] = true - store, err := build(ctx, env, m.Type, m.Config) + // Mirrors are best-effort write-only copies whose failures are logged + // and dropped; never pay on-demand ref-fetch network probes for them. + mirrorEnv := env + mirrorEnv.RefFetcher = nil + store, err := build(ctx, mirrorEnv, m.Type, m.Config) if err != nil { return nil, fmt.Errorf("checkpoints.mirrors[%d]: %w", i, err) } diff --git a/cmd/entire/cli/checkpoint/persistent.go b/cmd/entire/cli/checkpoint/persistent.go index 6d82820c75..67d135219c 100644 --- a/cmd/entire/cli/checkpoint/persistent.go +++ b/cmd/entire/cli/checkpoint/persistent.go @@ -1736,6 +1736,9 @@ func (s *GitStore) backfillSummary(ctx context.Context, checkpointID id.Checkpoi // // Returns ErrCheckpointNotFound if the checkpoint doesn't exist. func (s *GitStore) backfillTranscript(ctx context.Context, opts UpdateOptions) error { + if err := ctx.Err(); err != nil { + return err //nolint:wrapcheck // Propagating context cancellation + } if opts.CheckpointID.IsEmpty() { return errors.New("invalid update options: checkpoint ID is required") } diff --git a/cmd/entire/cli/checkpoint/refs_store.go b/cmd/entire/cli/checkpoint/refs_store.go index a8849b965d..756fd5778b 100644 --- a/cmd/entire/cli/checkpoint/refs_store.go +++ b/cmd/entire/cli/checkpoint/refs_store.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "strconv" + "sync" "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/plumbing" @@ -34,6 +35,17 @@ type gitRefsStore struct { blobFetcher BlobFetchFunc refFetcher RefFetchFunc + + // fetchFailureMu guards fetchFailure: the first transport-level ref-fetch + // failure, memoized for the store's lifetime so a loop over N missing refs + // (e.g. a stop hook finalizing every checkpoint of a turn) pays a dead — + // or too-slow-for-the-budget — network once instead of N times. Genuine + // remote absence is per-ref and + // is never memoized. The memo never clears — safe because every + // fetcher-wired store today is opened per command/hook invocation; a + // long-lived fetcher-wired store would need an expiry before reusing this. + fetchFailureMu sync.Mutex + fetchFailure error } // newGitRefsStore constructs the per-checkpoint-ref store for a repository. @@ -70,8 +82,18 @@ func (s *gitRefsStore) Write(ctx context.Context, req WriteRequest) error { } // refBase resolves a checkpoint ref's current tip commit (the parent for the -// next write) and subtree object (the checkpoint's current contents). A missing -// ref yields (ZeroHash, nil) so the next write becomes an orphan commit. +// next write) and subtree object (the checkpoint's current contents) with a +// LOCAL-ONLY lookup. A missing ref yields (ZeroHash, nil) so the next write +// becomes an orphan commit — correct for creates, whose ref never exists yet +// (locally or remotely); probing the remote would add a doomed round-trip to +// every condensation and, with a fetcher configured, fail offline writes. +// Backfills, which target an existing checkpoint, use refBaseForBackfill +// instead. Migration (migrate.go) also uses refBase deliberately: it imports +// from the LOCAL v1 branch and must never probe the remote, even though its +// target ref may already exist. One writeSession caller does target an +// existing checkpoint — attach, which adds a session to it — but attach +// pre-fetches and verifies the ref's presence itself (refreshCheckpoint) +// before writing, so the local-only probe is safe there too. func (s *gitRefsStore) refBase(cid id.CheckpointID) (plumbing.Hash, *object.Tree, error) { refName, err := RefName(cid) if err != nil { @@ -86,6 +108,35 @@ func (s *gitRefsStore) refBase(cid id.CheckpointID) (plumbing.Hash, *object.Tree // rather than silently starting a fresh orphan history over the ref. return plumbing.ZeroHash, nil, fmt.Errorf("resolve checkpoint ref %s: %w", refName, err) } + return s.refTip(cid, ref) +} + +// refBaseForBackfill resolves like refBase, but a ref missing locally is +// first fetched once from the remote (resolveRefMaybeFetch) when a fetcher is +// configured: a backfill targets an EXISTING checkpoint that may have been +// written or migrated on another machine, and declaring it absent without +// looking remotely diverges from the read path — the backfill would be +// handled as targeting a nonexistent checkpoint while reads, which DO fetch, +// serve the refs copy, leaving the backfilled data permanently invisible. +// A ref absent even after the fetch yields (ZeroHash, nil), which the +// backfill helpers report as ErrCheckpointNotFound — the signal that the +// checkpoint does not exist in this backend. A fetch FAILURE is returned +// as-is: transient unavailability must never masquerade as absence, because +// a caller or routing layer acting on a false "absent" would misdirect the +// backfill (e.g. onto a stale copy in another backend) instead of retrying. +func (s *gitRefsStore) refBaseForBackfill(ctx context.Context, cid id.CheckpointID) (plumbing.Hash, *object.Tree, error) { + ref, err := s.resolveRefMaybeFetch(ctx, cid) + if errors.Is(err, plumbing.ErrReferenceNotFound) { + return plumbing.ZeroHash, nil, nil // genuinely absent → backfill reports not-found + } + if err != nil { + return plumbing.ZeroHash, nil, err + } + return s.refTip(cid, ref) +} + +// refTip reads the commit and tree at a resolved checkpoint ref. +func (s *gitRefsStore) refTip(cid id.CheckpointID, ref *plumbing.Reference) (plumbing.Hash, *object.Tree, error) { commit, err := s.repo.CommitObject(ref.Hash()) if err != nil { return plumbing.ZeroHash, nil, fmt.Errorf("read checkpoint commit %s: %w", ref.Hash(), err) @@ -161,11 +212,14 @@ func (s *gitRefsStore) writeSession(ctx context.Context, opts WriteOptions) erro } func (s *gitRefsStore) backfillTranscript(ctx context.Context, opts UpdateOptions) error { + if err := ctx.Err(); err != nil { + return err //nolint:wrapcheck // Propagating context cancellation + } if opts.CheckpointID.IsEmpty() { return errors.New("invalid update options: checkpoint ID is required") } - parentHash, existing, err := s.refBase(opts.CheckpointID) + parentHash, existing, err := s.refBaseForBackfill(ctx, opts.CheckpointID) if err != nil { return err } @@ -192,7 +246,7 @@ func (s *gitRefsStore) backfillSummary(ctx context.Context, checkpointID id.Chec return err //nolint:wrapcheck // Propagating context cancellation } - parentHash, existing, err := s.refBase(checkpointID) + parentHash, existing, err := s.refBaseForBackfill(ctx, checkpointID) if err != nil { return err } @@ -216,7 +270,7 @@ func (s *gitRefsStore) backfillAttribution(ctx context.Context, checkpointID id. return err //nolint:wrapcheck // Propagating context cancellation } - parentHash, existing, err := s.refBase(checkpointID) + parentHash, existing, err := s.refBaseForBackfill(ctx, checkpointID) if err != nil { return err } @@ -284,7 +338,33 @@ func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.Checkpoi if s.refFetcher == nil { return nil, err //nolint:wrapcheck // genuinely absent; caller maps ErrReferenceNotFound to ErrCheckpointNotFound } + s.fetchFailureMu.Lock() + priorFailure := s.fetchFailure + s.fetchFailureMu.Unlock() + if priorFailure != nil { + // Note the cause may name a DIFFERENT ref — it is the first failure + // of this operation, remembered so the outage is paid once. + return nil, fmt.Errorf("fetch checkpoint ref %s: skipped, an earlier checkpoint-ref fetch already failed in this operation: %w", refName, priorFailure) + } if fetchErr := s.refFetcher(ctx, refName); fetchErr != nil { + if errors.Is(fetchErr, plumbing.ErrReferenceNotFound) { + // The fetcher probed the remote and it genuinely lacks this ref + // (remote.FetchCheckpointRef's absence signal) — absence, not a + // failure, and per-ref, so it is not memoized. + logging.Debug(ctx, "git-refs: remote has no such checkpoint ref", + slog.String("ref", refName.String())) + return nil, plumbing.ErrReferenceNotFound + } + // Memoize only network verdicts: a cancellation originating from the + // CALLER's context says nothing about the remote and must not poison + // later fetches on this store. + if ctx.Err() == nil { + s.fetchFailureMu.Lock() + if s.fetchFailure == nil { + s.fetchFailure = fetchErr + } + s.fetchFailureMu.Unlock() + } logging.Debug(ctx, "git-refs: on-demand checkpoint ref fetch failed", slog.String("ref", refName.String()), slog.String("error", fetchErr.Error())) return nil, fmt.Errorf("fetch checkpoint ref %s: %w", refName, fetchErr) diff --git a/cmd/entire/cli/checkpoint/refs_store_test.go b/cmd/entire/cli/checkpoint/refs_store_test.go index 3cd101504c..f3b5fed363 100644 --- a/cmd/entire/cli/checkpoint/refs_store_test.go +++ b/cmd/entire/cli/checkpoint/refs_store_test.go @@ -2,6 +2,7 @@ package checkpoint import ( "context" + "fmt" "testing" git "github.com/go-git/go-git/v6" @@ -118,6 +119,276 @@ func TestGitRefsStore_OnDemandRefFetch_FailurePropagates(t *testing.T) { }) } +// TestGitRefsStore_BackfillFetchesMissingRef: a backfill targets an EXISTING +// checkpoint, which may have been written or migrated on another machine — so +// like reads, backfills must on-demand fetch a ref that is missing locally +// before declaring the checkpoint absent. Otherwise the backfill is handled +// as targeting a nonexistent checkpoint while reads — which DO fetch — serve +// the refs copy, and the backfilled data is permanently invisible. +func TestGitRefsStore_BackfillFetchesMissingRef(t *testing.T) { + t.Parallel() + ctx := context.Background() + + backfills := map[string]struct { + makeReq func(cid id.CheckpointID) WriteRequest + verify func(t *testing.T, store *gitRefsStore, cid id.CheckpointID) + }{ + "summary": { + makeReq: func(cid id.CheckpointID) WriteRequest { + return SessionSummary{CheckpointID: cid, Summary: &Summary{Intent: "fetched intent"}} + }, + verify: func(t *testing.T, store *gitRefsStore, cid id.CheckpointID) { + t.Helper() + meta, err := store.ReadSessionMetadata(context.Background(), cid, 0) + require.NoError(t, err) + require.NotNil(t, meta.Summary) + assert.Equal(t, "fetched intent", meta.Summary.Intent) + }, + }, + "transcript": { + makeReq: func(cid id.CheckpointID) WriteRequest { + return SessionTranscript{ + CheckpointID: cid, + SessionID: "sess-1", + Transcript: redact.AlreadyRedacted([]byte("finalized")), + } + }, + verify: func(t *testing.T, store *gitRefsStore, cid id.CheckpointID) { + t.Helper() + content, err := store.ReadSessionContent(context.Background(), cid, 0) + require.NoError(t, err) + assert.Equal(t, []byte("finalized"), content.Transcript) + }, + }, + "attribution": { + makeReq: func(cid id.CheckpointID) WriteRequest { + return CheckpointAttribution{CheckpointID: cid, Attribution: &Attribution{AgentLines: 3}} + }, + verify: func(t *testing.T, store *gitRefsStore, cid id.CheckpointID) { + t.Helper() + summary, err := store.Read(context.Background(), cid) + require.NoError(t, err) + require.NotNil(t, summary) + require.NotNil(t, summary.CombinedAttribution) + assert.Equal(t, 3, summary.CombinedAttribution.AgentLines) + }, + }, + } + + for name, tc := range backfills { + t.Run(name, func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + cid := id.MustCheckpointID("a1b2c3d4e5f6") + refsWrite(t, store, cid, "sess-1", "transcript") + + ref, err := store.repo.Reference(mustRefName(t, cid), true) + require.NoError(t, err) + commitHash := ref.Hash() + require.NoError(t, store.repo.Storer.RemoveReference(mustRefName(t, cid))) + + fetched := 0 + store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error { + fetched++ + return store.repo.Storer.SetReference(plumbing.NewHashReference(rn, commitHash)) + }) + + require.NoError(t, store.Write(ctx, tc.makeReq(cid)), + "a backfill must fetch the missing ref instead of declaring the checkpoint absent") + assert.Equal(t, 1, fetched, "fetcher invoked once for the missing ref") + + // The write must have landed on the fetched history, not orphaned + // over it: the new tip's parent is the pre-removal commit. + newRef, err := store.repo.Reference(mustRefName(t, cid), true) + require.NoError(t, err) + newTip, err := store.repo.CommitObject(newRef.Hash()) + require.NoError(t, err) + require.Len(t, newTip.ParentHashes, 1) + assert.Equal(t, commitHash, newTip.ParentHashes[0], + "the backfill commit must parent on the fetched tip") + + tc.verify(t, store, cid) + }) + } +} + +// TestGitRefsStore_BackfillLocalRefNeverFetches pins the zero-cost claim: a +// backfill whose ref exists locally must not touch the remote — otherwise +// every summary/attribution/transcript backfill pays a network round-trip and +// offline finalization breaks. +func TestGitRefsStore_BackfillLocalRefNeverFetches(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + cid := id.MustCheckpointID("a1b2c3d4e5f6") + refsWrite(t, store, cid, "sess-1", "transcript") + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + t.Error("a backfill with a locally-present ref must not fetch") + return nil + }) + + require.NoError(t, store.Write(context.Background(), SessionSummary{ + CheckpointID: cid, + Summary: &Summary{Intent: "local"}, + })) +} + +// TestGitRefsStore_BackfillFetchFailureAborts: a failed fetch is a transient +// availability problem, not evidence of absence. The backfill must surface it +// as a real error — NOT ErrCheckpointNotFound, which callers treat as "the +// checkpoint does not exist in this backend", a signal a routing layer may +// act on to select a different backend (forking the write onto a stale copy). +func TestGitRefsStore_BackfillFetchFailureAborts(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + return assert.AnError // offline / network failure + }) + + err := store.Write(context.Background(), SessionSummary{ + CheckpointID: id.MustCheckpointID("ffffffffffff"), + Summary: &Summary{Intent: "must not land"}, + }) + require.ErrorIs(t, err, assert.AnError, "the fetch failure must surface") + require.NotErrorIs(t, err, ErrCheckpointNotFound, + "a fetch failure must not read as absence") +} + +// TestGitRefsStore_RemoteAbsenceFromFetcherIsNotFound pins the classification +// chain for a fetcher that reports "the remote has no such ref" by wrapping +// plumbing.ErrReferenceNotFound (remote.FetchCheckpointRef's absence signal): +// both backfills and reads must treat it as checkpoint-not-found, not as a +// hard failure. +func TestGitRefsStore_RemoteAbsenceFromFetcherIsNotFound(t *testing.T) { + t.Parallel() + ctx := context.Background() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error { + return fmt.Errorf("checkpoint ref %s not found on origin: %w", rn, plumbing.ErrReferenceNotFound) + }) + + err := store.Write(ctx, SessionSummary{ + CheckpointID: id.MustCheckpointID("ffffffffffff"), + Summary: &Summary{Intent: "orphan"}, + }) + require.ErrorIs(t, err, ErrCheckpointNotFound, "remote absence must classify as not-found for backfills") + + summary, err := store.Read(ctx, id.MustCheckpointID("ffffffffffff")) + require.NoError(t, err) + assert.Nil(t, summary, "remote absence must classify as not-found for reads") +} + +// TestGitRefsStore_FetchFailureMemoized: a transport-level fetch failure is +// remembered for the store's lifetime, so a loop backfilling N checkpoints on +// a dead network pays the outage once instead of N times (stop hooks finalize +// every checkpoint of a turn). The memoized error stays a hard error — never +// absence. Genuine remote absence is NOT memoized (per-ref, not an outage). +func TestGitRefsStore_FetchFailureMemoized(t *testing.T) { + t.Parallel() + ctx := context.Background() + + t.Run("transport failure fetched once", func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + calls := 0 + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + calls++ + return assert.AnError + }) + + for _, cid := range []string{"aaaaaaaaaaaa", "bbbbbbbbbbbb"} { + err := store.Write(ctx, SessionSummary{ + CheckpointID: id.MustCheckpointID(cid), + Summary: &Summary{Intent: "x"}, + }) + require.ErrorIs(t, err, assert.AnError) + require.NotErrorIs(t, err, ErrCheckpointNotFound) + } + assert.Equal(t, 1, calls, "the outage must be paid once, not per checkpoint") + }) + + t.Run("caller cancellation not memoized", func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + calls := 0 + cancelCtx, cancel := context.WithCancel(context.Background()) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + calls++ + cancel() // the CALLER's context dies mid-fetch (e.g. Ctrl-C) + return context.Canceled + }) + err := store.Write(cancelCtx, SessionSummary{ + CheckpointID: id.MustCheckpointID("aaaaaaaaaaaa"), + Summary: &Summary{Intent: "x"}, + }) + require.Error(t, err) + + // A later fetch on the same store must still run: the cancellation + // said nothing about the network. + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + calls++ + return assert.AnError + }) + err = store.Write(context.Background(), SessionSummary{ + CheckpointID: id.MustCheckpointID("bbbbbbbbbbbb"), + Summary: &Summary{Intent: "x"}, + }) + require.ErrorIs(t, err, assert.AnError) + assert.Equal(t, 2, calls, "a caller cancellation must not be memoized as a network failure") + }) + + t.Run("remote absence not memoized", func(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + calls := 0 + store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error { + calls++ + return fmt.Errorf("ref %s not on remote: %w", rn, plumbing.ErrReferenceNotFound) + }) + + for _, cid := range []string{"aaaaaaaaaaaa", "bbbbbbbbbbbb"} { + err := store.Write(ctx, SessionSummary{ + CheckpointID: id.MustCheckpointID(cid), + Summary: &Summary{Intent: "x"}, + }) + require.ErrorIs(t, err, ErrCheckpointNotFound) + } + assert.Equal(t, 2, calls, "absence is per-ref and must not suppress later fetches") + }) +} + +// TestGitRefsStore_BackfillAbsentAfterFetchIsNotFound pins the genuine-absence +// contract: a fetch that succeeds but restores no ref means the checkpoint +// really does not exist in this backend, and the backfill reports the +// not-found sentinel (which a routing layer may legitimately act on). +func TestGitRefsStore_BackfillAbsentAfterFetchIsNotFound(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + return nil // fetch "succeeds" but the remote has no such ref + }) + + err := store.Write(context.Background(), SessionSummary{ + CheckpointID: id.MustCheckpointID("ffffffffffff"), + Summary: &Summary{Intent: "orphan"}, + }) + require.ErrorIs(t, err, ErrCheckpointNotFound) +} + +// TestGitRefsStore_CreateNeverFetches pins the deliberate split: a create's +// ref never exists yet (locally or remotely), so writeSession must not probe +// the remote — fetch-on-create would add a doomed network round-trip to every +// condensation and break offline writes. +func TestGitRefsStore_CreateNeverFetches(t *testing.T) { + t.Parallel() + store := newRefsStore(t) + store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error { + t.Error("a create must never invoke the ref fetcher") + return nil + }) + + refsWrite(t, store, id.MustCheckpointID("a1b2c3d4e5f6"), "sess-new", "fresh transcript") +} + func TestGitRefsStore_WriteAllVariantsAndRead(t *testing.T) { t.Parallel() store := newRefsStore(t) diff --git a/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go b/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go new file mode 100644 index 0000000000..8cd045e775 --- /dev/null +++ b/cmd/entire/cli/checkpoint/remote/checkpoint_ref.go @@ -0,0 +1,124 @@ +package remote + +import ( + "bytes" + "context" + "fmt" + "strings" + "time" + + "github.com/go-git/go-git/v6/plumbing" +) + +// WriteProbeFetchBudget bounds the on-demand ref fetch performed by a +// BACKFILL's absence probe. Write paths run inside git hooks (post-commit, +// stop-time finalize) where a dead network must not stall the user's +// workflow for the read path's full fetch window; combined with the +// per-store failure memo in the git-refs store, a loop over N checkpoints +// pays a dead network once, briefly. +const WriteProbeFetchBudget = 15 * time.Second + +// readFetchTimeout bounds the interactive read-path fetch (unchanged from +// the historical FetchCheckpointRef behavior). +const readFetchTimeout = 2 * time.Minute + +// CheckpointFetchTarget returns the git remote (URL or name) that checkpoint +// data is fetched from. It prefers the effective URL resolved by FetchURL, +// which is the source of truth for checkpoint fetch location. If URL +// resolution fails, it falls back to the origin remote name so callers can +// still attempt a fetch. +func CheckpointFetchTarget(ctx context.Context) string { + target, _ := checkpointFetchTarget(ctx) + return target +} + +// checkpointFetchTarget is CheckpointFetchTarget plus whether the target is +// authoritative for checkpoint refs (see fetchURLAuthoritative). The bare +// "origin" fallbacks are non-authoritative: they exist so a fetch can still +// be attempted, not to certify where checkpoint refs live. +func checkpointFetchTarget(ctx context.Context) (string, bool) { + url, authoritative, err := fetchURLAuthoritative(ctx) + if err == nil && url != "" { + return url, authoritative + } + return "origin", false +} + +// FetchCheckpointRef fetches a single per-checkpoint ref +// (refs/entire/checkpoints//) from the checkpoint remote into the +// local ref of the same name, so the git-refs store can resolve a checkpoint +// written on another machine. +// +// Contract — absence is distinguishable from failure: +// - The remote genuinely lacking the ref returns an error wrapping +// plumbing.ErrReferenceNotFound (probed via ls-remote before fetching, +// because `git fetch` of a missing refspec fails indistinguishably from a +// transport error). Store probes classify this as "checkpoint not found", +// which write routing may legitimately act on. +// - Any transport-level failure (probe or fetch) is surfaced as a real +// error, never mapped to absence — a false "absent" would misdirect a +// backfill onto another backend instead of retrying. +func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error { + ctx, cancel := context.WithTimeout(ctx, readFetchTimeout) + defer cancel() + + fetchTarget, authoritative := checkpointFetchTarget(ctx) + + out, err := LsRemoteInDir(ctx, "", fetchTarget, ref.String()) + if err != nil { + // Redact: fetchTarget can be a remote URL with embedded credentials + // (CI origin URLs), and this error is logged and shown to users. + return fmt.Errorf("probe checkpoint ref %s on %s: %w", ref, RedactURL(fetchTarget), err) + } + if len(bytes.TrimSpace(out)) == 0 { + if !authoritative { + // The probe hit an origin FALLBACK while a checkpoint_remote is + // configured (or undeterminable) — a remote that may simply never + // host the configured checkpoint refs. Emptiness there proves + // nothing; classifying it as absence would silently drop backfills + // for checkpoints that exist on the real checkpoint remote. + return fmt.Errorf("checkpoint ref %s not visible on fallback remote %s, and the configured checkpoint remote could not be resolved; refusing to treat this as absence", ref, RedactURL(fetchTarget)) + } + return fmt.Errorf("checkpoint ref %s not found on %s: %w", ref, RedactURL(fetchTarget), plumbing.ErrReferenceNotFound) + } + + refSpec := "+" + ref.String() + ":" + ref.String() + if fetchOut, err := Fetch(ctx, FetchOptions{ + Remote: fetchTarget, + RefSpecs: []string{refSpec}, + NoTags: true, + }); err != nil { + // Fold git's own output into the error (redacted): a bare + // "exit status 128" is undebuggable in hook Warn logs. + msg := strings.TrimSpace(string(fetchOut)) + msg = strings.ReplaceAll(msg, fetchTarget, RedactURL(fetchTarget)) + if msg != "" { + return fmt.Errorf("fetch checkpoint ref %s from %s: %s: %w", ref, RedactURL(fetchTarget), msg, err) + } + return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, RedactURL(fetchTarget), err) + } + return nil +} + +// HookCheckpointRefFetcher returns the write-probe fetcher for git-hook +// contexts (post-commit attribution, stop-time transcript finalize): the +// bounded budget plus BatchMode SSH, so a passphrase-protected key can never +// prompt — or invisibly hang — inside a hook the user's git command is +// waiting on. +func HookCheckpointRefFetcher() func(context.Context, plumbing.ReferenceName) error { + bounded := BoundedCheckpointRefFetcher(WriteProbeFetchBudget) + return func(ctx context.Context, ref plumbing.ReferenceName) error { + return bounded(WithNonInteractiveSSH(ctx), ref) + } +} + +// BoundedCheckpointRefFetcher returns a RefFetchFunc-shaped fetcher whose +// per-call budget is capped at d, for wiring into write-path checkpoint +// stores (see WriteProbeFetchBudget). +func BoundedCheckpointRefFetcher(d time.Duration) func(context.Context, plumbing.ReferenceName) error { + return func(ctx context.Context, ref plumbing.ReferenceName) error { + ctx, cancel := context.WithTimeout(ctx, d) + defer cancel() + return FetchCheckpointRef(ctx, ref) + } +} diff --git a/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go b/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go new file mode 100644 index 0000000000..d07c1ed28d --- /dev/null +++ b/cmd/entire/cli/checkpoint/remote/checkpoint_ref_test.go @@ -0,0 +1,96 @@ +package remote + +import ( + "context" + "os/exec" + "testing" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/require" + + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +// checkpointRefFixture creates a work repo whose origin is a local bare repo +// holding (or not holding) a checkpoint ref, and chdirs into the work repo so +// fetch-target resolution finds origin. +func checkpointRefFixture(t *testing.T, withRef bool) (workDir string, ref plumbing.ReferenceName) { + t.Helper() + bareDir := t.TempDir() + out, err := exec.CommandContext(t.Context(), "git", "init", "--bare", bareDir).CombinedOutput() + require.NoError(t, err, "git init --bare: %s", out) + + workDir = t.TempDir() + testutil.InitRepo(t, workDir) + testutil.WriteFile(t, workDir, "f.txt", "content") + testutil.GitAdd(t, workDir, "f.txt") + testutil.GitCommit(t, workDir, "init") + out, err = exec.CommandContext(t.Context(), "git", "-C", workDir, "remote", "add", "origin", bareDir).CombinedOutput() + require.NoError(t, err, "git remote add: %s", out) + + ref = plumbing.ReferenceName("refs/entire/checkpoints/Z9/01KVBJCWYA4YW6J5M9GP655HZ9") + if withRef { + out, err = exec.CommandContext(t.Context(), "git", "-C", workDir, "push", "--quiet", "origin", "HEAD:"+ref.String()).CombinedOutput() + require.NoError(t, err, "git push checkpoint ref: %s", out) + } + + t.Chdir(workDir) + return workDir, ref +} + +// TestFetchCheckpointRef_RemoteMissingRefIsAbsence: a remote that does not +// have the requested checkpoint ref is ABSENCE, not a transport failure — the +// error must wrap plumbing.ErrReferenceNotFound so store probes (reads and +// backfill writes) classify it as "checkpoint not found" and, under kind +// routing, may legitimately fall through to another backend. Before this +// distinction, git fetch of a missing refspec failed like a network error, +// which made wiring a fetcher into write paths unsafe. +func TestFetchCheckpointRef_RemoteMissingRefIsAbsence(t *testing.T) { + _, ref := checkpointRefFixture(t, false) + + err := FetchCheckpointRef(context.Background(), ref) + require.Error(t, err) + require.ErrorIs(t, err, plumbing.ErrReferenceNotFound, + "a ref the remote does not have must classify as absence") +} + +// TestFetchCheckpointRef_PresentRefFetches: the ref exists on the remote but +// not locally; the fetch must create the local ref of the same name. +func TestFetchCheckpointRef_PresentRefFetches(t *testing.T) { + workDir, ref := checkpointRefFixture(t, true) + + require.NoError(t, FetchCheckpointRef(context.Background(), ref)) + + out, err := exec.CommandContext(t.Context(), "git", "-C", workDir, "show-ref", "--verify", ref.String()).CombinedOutput() + require.NoError(t, err, "fetched ref must exist locally: %s", out) +} + +// TestFetchCheckpointRef_FallbackTargetNeverClassifiesAbsence: when a +// checkpoint_remote is configured but cannot be resolved (unknown provider + +// an origin whose protocol can't be mapped), the probe runs against an origin +// FALLBACK that never hosts the configured checkpoint refs. Emptiness there +// must be a failure, not absence — absence would silently drop backfills for +// checkpoints that exist on the real checkpoint remote. +func TestFetchCheckpointRef_FallbackTargetNeverClassifiesAbsence(t *testing.T) { + workDir, ref := checkpointRefFixture(t, false) + testutil.WriteFile(t, workDir, ".entire/settings.json", + `{"enabled": true, "strategy_options": {"checkpoint_remote": {"provider": "bogusforge", "repo": "acme/checkpoints"}}}`) + + err := FetchCheckpointRef(context.Background(), ref) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound, + "emptiness on a non-authoritative fallback target must not classify as absence") +} + +// TestFetchCheckpointRef_UnreachableRemoteIsFailure: a transport-level +// failure (unreachable remote) must NOT classify as absence. +func TestFetchCheckpointRef_UnreachableRemoteIsFailure(t *testing.T) { + workDir, ref := checkpointRefFixture(t, false) + out, err := exec.CommandContext(t.Context(), "git", "-C", workDir, "remote", "set-url", "origin", workDir+"/nonexistent-remote").CombinedOutput() + require.NoError(t, err, "%s", out) + + err = FetchCheckpointRef(context.Background(), ref) + require.Error(t, err) + require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound, + "a transport failure must stay distinguishable from absence") +} diff --git a/cmd/entire/cli/checkpoint/remote/git.go b/cmd/entire/cli/checkpoint/remote/git.go index 1de3ef55fb..68f6e823dd 100644 --- a/cmd/entire/cli/checkpoint/remote/git.go +++ b/cmd/entire/cli/checkpoint/remote/git.go @@ -3,6 +3,7 @@ package remote import ( "context" "encoding/base64" + "errors" "fmt" "log/slog" "os" @@ -461,6 +462,15 @@ func lsRemote(ctx context.Context, dir, remote string, patterns ...string) ([]by disableTerminalPrompt(cmd) out, err := cmd.Output() if err != nil { + // Fold git's stderr into the error (redacted): a bare "exit status + // 128" leaves auth, DNS, and missing-repo failures indistinguishable. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + if msg := strings.TrimSpace(string(exitErr.Stderr)); msg != "" { + msg = strings.ReplaceAll(msg, remote, RedactURL(remote)) + return out, fmt.Errorf("git ls-remote: %w: %s", err, msg) + } + } return out, fmt.Errorf("git ls-remote: %w", err) } return out, nil diff --git a/cmd/entire/cli/checkpoint/remote/util.go b/cmd/entire/cli/checkpoint/remote/util.go index aad8668cd7..f57122f504 100644 --- a/cmd/entire/cli/checkpoint/remote/util.go +++ b/cmd/entire/cli/checkpoint/remote/util.go @@ -39,6 +39,18 @@ type FetchURLOptions struct { // If ENTIRE_CHECKPOINT_TOKEN is set and a checkpoint remote is configured, HTTPS is // forced so the token can be used even when origin is configured via SSH. func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { + url, _, err := fetchURLAuthoritative(ctx, opts...) + return url, err +} + +// fetchURLAuthoritative is FetchURL plus whether the returned URL is +// authoritative for checkpoint refs. It is false exactly when a +// checkpoint_remote IS configured (or cannot be determined) but resolution +// fell back to the origin URL — a remote that by construction does not host +// the configured checkpoint refs. Callers that classify "ref absent on the +// remote" (FetchCheckpointRef's ls-remote probe) must not treat emptiness on +// a non-authoritative target as absence. +func fetchURLAuthoritative(ctx context.Context, opts ...FetchURLOptions) (string, bool, error) { var opt FetchURLOptions if len(opts) > 0 { opt = opts[0] @@ -69,17 +81,20 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { if err != nil { if originURL != "" { logFallback(ctx, "fetch", originURL, "load settings", err) - return originURL, nil + // Settings unreadable → checkpoint_remote unknown; conservative: + // do not certify origin as authoritative for checkpoint refs. + return originURL, false, nil } - return "", fmt.Errorf("load settings: %w", err) + return "", false, fmt.Errorf("load settings: %w", err) } config := s.GetCheckpointRemote() if config == nil { if originURL == "" { - return "", fmt.Errorf("no fetch URL found: %w", originErr) + return "", false, fmt.Errorf("no fetch URL found: %w", originErr) } - return originURL, nil + // No checkpoint_remote configured: origin IS the checkpoint host. + return originURL, true, nil } if withToken { @@ -90,25 +105,25 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { Host: host, }, config) if err == nil { - return checkpointURL, nil + return checkpointURL, true, nil } } // In token-based execution path, short-circuit to avoid additional // change in protocol. if originURL != "" { - return originURL, nil + return originURL, false, nil } } if originURL == "" { - return "", fmt.Errorf("no fetch URL found: %w", originErr) + return "", false, fmt.Errorf("no fetch URL found: %w", originErr) } info, err := ParseURL(originURL) if err != nil { logFallback(ctx, "fetch", originURL, "parse origin remote URL", err) - return originURL, nil + return originURL, false, nil } checkpointURL, err := deriveCheckpointURLFromInfo(info, config) @@ -118,13 +133,13 @@ func FetchURL(ctx context.Context, opts ...FetchURLOptions) (string, error) { // provider). Honor the configured checkpoint_remote by targeting the // provider's canonical host over HTTPS rather than falling back to origin. if providerURL, ok := resolveProviderCheckpointURL(config, opt.WorktreeRoot); ok { - return providerURL, nil + return providerURL, true, nil } logFallback(ctx, "fetch", originURL, "derive checkpoint remote URL", err) - return originURL, nil + return originURL, false, nil } - return checkpointURL, nil + return checkpointURL, true, nil } // PushURL returns the effective checkpoint push URL for the current repository. diff --git a/cmd/entire/cli/explain.go b/cmd/entire/cli/explain.go index 48f1d9a260..8f4c1a1e0f 100644 --- a/cmd/entire/cli/explain.go +++ b/cmd/entire/cli/explain.go @@ -23,6 +23,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/agent/types" "github.com/entireio/cli/cmd/entire/cli/checkpoint" "github.com/entireio/cli/cmd/entire/cli/checkpoint/id" + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/palette" @@ -719,7 +720,12 @@ func runExplainCheckpointWithLookup(ctx context.Context, w, errW io.Writer, chec return err } stopLoad(false) // generation prints its own progress to w/errW - writeStores, openErr := checkpoint.Open(ctx, lookup.repo, checkpoint.OpenOptions{}) + // RefFetcher: the summary backfill's absence probe fetches a ref that + // exists remotely but not locally (written/migrated on another + // machine), bounded by the write-probe budget. + writeStores, openErr := checkpoint.Open(ctx, lookup.repo, checkpoint.OpenOptions{ + RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget), + }) if openErr != nil { return fmt.Errorf("open checkpoint store: %w", openErr) } diff --git a/cmd/entire/cli/git_operations.go b/cmd/entire/cli/git_operations.go index 195103485b..1580b5ca45 100644 --- a/cmd/entire/cli/git_operations.go +++ b/cmd/entire/cli/git_operations.go @@ -462,37 +462,18 @@ func FetchMetadataFromCheckpointRemote(ctx context.Context) error { } // resolveCheckpointFetchTarget returns the fetch target for checkpoint data. -// It prefers the effective URL resolved by checkpoint/remote.FetchURL, which is -// the source of truth for checkpoint fetch location. If URL resolution fails, it -// falls back to the origin remote name so callers can still attempt a fetch. +// Thin alias for remote.CheckpointFetchTarget (the single source of truth). func resolveCheckpointFetchTarget(ctx context.Context) string { - url, err := remote.FetchURL(ctx) - if err == nil && url != "" { - return url - } - return "origin" + return remote.CheckpointFetchTarget(ctx) } -// FetchCheckpointRef fetches a single per-checkpoint ref (refs/entire/checkpoints/ -// /) from the checkpoint remote into the local ref of the same name, -// so the git-refs store can resolve a checkpoint written on another machine. -// Best-effort: the caller treats a fetch failure as "checkpoint not found". +// FetchCheckpointRef fetches a single per-checkpoint ref from the checkpoint +// remote. Thin alias for remote.FetchCheckpointRef, kept so existing cli-side +// call sites and OpenOptions wiring stay unchanged; see that function for the +// absence-vs-failure contract (remote-lacks-ref wraps +// plumbing.ErrReferenceNotFound; transport failures surface as-is). func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error { - ctx, cancel := context.WithTimeout(ctx, 2*time.Minute) - defer cancel() - - fetchTarget := resolveCheckpointFetchTarget(ctx) - refSpec := "+" + ref.String() + ":" + ref.String() - if _, err := remote.Fetch(ctx, remote.FetchOptions{ - Remote: fetchTarget, - RefSpecs: []string{refSpec}, - NoTags: true, - }); err != nil { - // Redact: fetchTarget can be a remote URL with embedded credentials - // (CI origin URLs), and this error is logged and shown to users. - return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, remote.RedactURL(fetchTarget), err) - } - return nil + return remote.FetchCheckpointRef(ctx, ref) //nolint:wrapcheck // thin alias; the remote error carries full context } // FetchBlobsByHash fetches specific blob objects from the remote by their SHA-1 hashes. diff --git a/cmd/entire/cli/strategy/manual_commit_hooks.go b/cmd/entire/cli/strategy/manual_commit_hooks.go index fbe29dbef6..17f14a86d5 100644 --- a/cmd/entire/cli/strategy/manual_commit_hooks.go +++ b/cmd/entire/cli/strategy/manual_commit_hooks.go @@ -21,6 +21,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/checkpoint" "github.com/entireio/cli/cmd/entire/cli/checkpoint/id" + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" "github.com/entireio/cli/cmd/entire/cli/checkpointpolicy" "github.com/entireio/cli/cmd/entire/cli/gitops" "github.com/entireio/cli/cmd/entire/cli/interactive" @@ -1063,7 +1064,12 @@ func (s *ManualCommitStrategy) updateCombinedAttributionForCheckpoint( repoDir string, ) error { logCtx := logging.WithComponent(ctx, "attribution") - stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{}) + // RefFetcher: the attribution backfill's absence probe fetches a ref that + // exists remotely but not locally. Bounded budget + the store's failure + // memo keep a dead network from stalling the post-commit hook. + stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{ + RefFetcher: remote.HookCheckpointRefFetcher(), + }) if err != nil { return fmt.Errorf("open checkpoint store: %w", err) } @@ -2897,7 +2903,13 @@ func (s *ManualCommitStrategy) finalizeAllTurnCheckpoints(ctx context.Context, s // Post-commit emits regex-only blobs; the writer joins + redacts // via checkpoint.redactedJoinedPrompts. OPF runs later, once per // push, in the pre-push rewrite path. - stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{}) + // RefFetcher: the transcript finalize targets existing checkpoints whose + // refs may live only on the remote (resumed/adopted multi-machine + // sessions). Bounded budget + the store's failure memo keep a dead + // network from stalling the stop hook N times. + stores, err := checkpoint.Open(ctx, repo, checkpoint.OpenOptions{ + RefFetcher: remote.HookCheckpointRefFetcher(), + }) if err != nil { logging.Warn(logCtx, "finalize: failed to open checkpoint store", slog.String("error", err.Error()), diff --git a/docs/architecture/ref-checkpoint-backend.md b/docs/architecture/ref-checkpoint-backend.md index 31f4988113..1b0a8556e3 100644 --- a/docs/architecture/ref-checkpoint-backend.md +++ b/docs/architecture/ref-checkpoint-backend.md @@ -69,7 +69,7 @@ The git-refs store (`gitRefsStore`, `checkpoint/refs_store.go`) shares the check Every persistent write (`WriteSession`, and the `Backfill*` operations for transcript / summary / attribution) follows the same shape: -1. **Resolve the ref's current tip** (`refBase`). A missing ref → `(ZeroHash, nil)`, so the first write to a checkpoint becomes an **orphan commit**. A real lookup failure (IO/corruption) is surfaced, never silently treated as "new checkpoint". +1. **Resolve the ref's current tip.** Creates (`WriteSession`) use the local-only `refBase`: a missing ref → `(ZeroHash, nil)`, so the first write to a checkpoint becomes an **orphan commit**. The `Backfill*` operations use `refBaseForBackfill`, which first on-demand fetches a locally-missing ref (when a ref fetcher is configured — the checkpoint may have been written or migrated on another machine); a ref still absent after the fetch surfaces as `ErrCheckpointNotFound` rather than orphaning, and a fetch failure surfaces as a real error, never as absence. A real lookup failure (IO/corruption) is surfaced, never silently treated as "new checkpoint". 2. **Build the updated checkpoint subtree** from the existing tree plus the new content (shared `treeWriter` logic). 3. **Create a commit** with the current tip as parent (orphan on first write, parented thereafter), so each checkpoint accretes its **own per-checkpoint history**. 4. **Point the ref at the new commit** (`setRef`) and **enqueue it for push**. @@ -111,9 +111,9 @@ All checkpoint-ref pushes are **fast-forward-only — never a force push.** Ther When a push *is* rejected as non-fast-forward — genuine divergence, e.g. the same checkpoint was written on two machines — recovery **fetches the remote ref and replays the local-only commits on top** (`fetchAndRebaseRefCommon`), then retries. After the replay the local ref is a fast-forward over the remote, so the retry is *still* non-force and the remote commit is preserved as an ancestor rather than overwritten. A genuine cherry-pick conflict (both sides rewrote the same file, e.g. root `metadata.json`) leaves the ref queued — degrading to the safe state, never forcing. -### On-demand fetch for reads +### On-demand fetch (reads and backfill writes) -A checkpoint written on another machine has no local ref. When a read misses locally and a **ref fetcher** is configured, `resolveRefMaybeFetch` fetches that one ref from the remote and retries once. It carefully distinguishes: +A checkpoint written on another machine has no local ref. When a read — or a backfill write's base resolution (`refBaseForBackfill`) — misses locally and a **ref fetcher** is configured, `resolveRefMaybeFetch` fetches that one ref from the remote and retries once. It carefully distinguishes: - **genuinely absent** (remote has no such checkpoint) → maps to `ErrCheckpointNotFound`; - **a real failure** (IO, network, context cancellation) → returned as-is, never swallowed as "not found".