Skip to content

Commit b33b3c2

Browse files
peyton-altclaude
andcommitted
feat(checkpoint): wire the backfill ref fetcher into write paths
Completes the #1824 fix — previously the fetch-probing backfill base was store-layer plumbing with no production caller wiring a RefFetcher. The two prerequisites the review named are now in place: - remote.FetchCheckpointRef (moved from the cli package so strategy hooks can wire it; cli keeps thin aliases) probes with ls-remote before fetching: a ref the remote genuinely lacks returns an error wrapping plumbing.ErrReferenceNotFound (absence — safe fallthrough under kind routing), while transport failures on either leg surface as-is. git fetch of a missing refspec fails indistinguishably from a network error, which is why wiring was unsafe before this discrimination existed. - Write probes are budget-bounded (remote.WriteProbeFetchBudget, 15s via remote.BoundedCheckpointRefFetcher) and the git-refs store memoizes the first transport-level fetch failure per store, so a stop hook finalizing N checkpoints on a dead network pays the outage once, briefly. Remote absence is per-ref and never memoized. Wired at the three backfill write sites: explain --generate's write store, the stop-hook transcript finalize, and post-commit attribution. The carry-forward and other creates stay unwired — creates never probe. Tests: remote-level absence/present/transport-failure fixtures against a local bare origin; store-level absence classification chain-through and failure-memo behavior (transport memoized once, absence never). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 2b4b736 commit b33b3c2

7 files changed

Lines changed: 297 additions & 36 deletions

File tree

cmd/entire/cli/checkpoint/refs_store.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"log/slog"
88
"strconv"
9+
"sync"
910

1011
"github.com/go-git/go-git/v6"
1112
"github.com/go-git/go-git/v6/plumbing"
@@ -34,6 +35,14 @@ type gitRefsStore struct {
3435

3536
blobFetcher BlobFetchFunc
3637
refFetcher RefFetchFunc
38+
39+
// fetchFailureMu guards fetchFailure: the first transport-level ref-fetch
40+
// failure, memoized for the store's lifetime so a loop over N missing refs
41+
// (e.g. a stop hook finalizing every checkpoint of a turn) pays a dead
42+
// network once instead of N times. Genuine remote absence is per-ref and
43+
// is never memoized.
44+
fetchFailureMu sync.Mutex
45+
fetchFailure error
3746
}
3847

3948
// newGitRefsStore constructs the per-checkpoint-ref store for a repository.
@@ -323,7 +332,26 @@ func (s *gitRefsStore) resolveRefMaybeFetch(ctx context.Context, cid id.Checkpoi
323332
if s.refFetcher == nil {
324333
return nil, err //nolint:wrapcheck // genuinely absent; caller maps ErrReferenceNotFound to ErrCheckpointNotFound
325334
}
335+
s.fetchFailureMu.Lock()
336+
priorFailure := s.fetchFailure
337+
s.fetchFailureMu.Unlock()
338+
if priorFailure != nil {
339+
return nil, fmt.Errorf("fetch checkpoint ref %s: skipping after earlier fetch failure: %w", refName, priorFailure)
340+
}
326341
if fetchErr := s.refFetcher(ctx, refName); fetchErr != nil {
342+
if errors.Is(fetchErr, plumbing.ErrReferenceNotFound) {
343+
// The fetcher probed the remote and it genuinely lacks this ref
344+
// (remote.FetchCheckpointRef's absence signal) — absence, not a
345+
// failure, and per-ref, so it is not memoized.
346+
logging.Debug(ctx, "git-refs: remote has no such checkpoint ref",
347+
slog.String("ref", refName.String()))
348+
return nil, plumbing.ErrReferenceNotFound
349+
}
350+
s.fetchFailureMu.Lock()
351+
if s.fetchFailure == nil {
352+
s.fetchFailure = fetchErr
353+
}
354+
s.fetchFailureMu.Unlock()
327355
logging.Debug(ctx, "git-refs: on-demand checkpoint ref fetch failed",
328356
slog.String("ref", refName.String()), slog.String("error", fetchErr.Error()))
329357
return nil, fmt.Errorf("fetch checkpoint ref %s: %w", refName, fetchErr)

cmd/entire/cli/checkpoint/refs_store_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package checkpoint
22

33
import (
44
"context"
5+
"fmt"
56
"testing"
67

78
git "github.com/go-git/go-git/v6"
@@ -252,6 +253,79 @@ func TestGitRefsStore_BackfillFetchFailureAborts(t *testing.T) {
252253
"a fetch failure must not read as absence")
253254
}
254255

256+
// TestGitRefsStore_RemoteAbsenceFromFetcherIsNotFound pins the classification
257+
// chain for a fetcher that reports "the remote has no such ref" by wrapping
258+
// plumbing.ErrReferenceNotFound (remote.FetchCheckpointRef's absence signal):
259+
// both backfills and reads must treat it as checkpoint-not-found, not as a
260+
// hard failure.
261+
func TestGitRefsStore_RemoteAbsenceFromFetcherIsNotFound(t *testing.T) {
262+
t.Parallel()
263+
ctx := context.Background()
264+
store := newRefsStore(t)
265+
store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error {
266+
return fmt.Errorf("checkpoint ref %s not found on origin: %w", rn, plumbing.ErrReferenceNotFound)
267+
})
268+
269+
err := store.Write(ctx, SessionSummary{
270+
CheckpointID: id.MustCheckpointID("ffffffffffff"),
271+
Summary: &Summary{Intent: "orphan"},
272+
})
273+
require.ErrorIs(t, err, ErrCheckpointNotFound, "remote absence must classify as not-found for backfills")
274+
275+
summary, err := store.Read(ctx, id.MustCheckpointID("ffffffffffff"))
276+
require.NoError(t, err)
277+
assert.Nil(t, summary, "remote absence must classify as not-found for reads")
278+
}
279+
280+
// TestGitRefsStore_FetchFailureMemoized: a transport-level fetch failure is
281+
// remembered for the store's lifetime, so a loop backfilling N checkpoints on
282+
// a dead network pays the outage once instead of N times (stop hooks finalize
283+
// every checkpoint of a turn). The memoized error stays a hard error — never
284+
// absence. Genuine remote absence is NOT memoized (per-ref, not an outage).
285+
func TestGitRefsStore_FetchFailureMemoized(t *testing.T) {
286+
t.Parallel()
287+
ctx := context.Background()
288+
289+
t.Run("transport failure fetched once", func(t *testing.T) {
290+
t.Parallel()
291+
store := newRefsStore(t)
292+
calls := 0
293+
store.SetRefFetcher(func(_ context.Context, _ plumbing.ReferenceName) error {
294+
calls++
295+
return assert.AnError
296+
})
297+
298+
for _, cid := range []string{"aaaaaaaaaaaa", "bbbbbbbbbbbb"} {
299+
err := store.Write(ctx, SessionSummary{
300+
CheckpointID: id.MustCheckpointID(cid),
301+
Summary: &Summary{Intent: "x"},
302+
})
303+
require.ErrorIs(t, err, assert.AnError)
304+
require.NotErrorIs(t, err, ErrCheckpointNotFound)
305+
}
306+
assert.Equal(t, 1, calls, "the outage must be paid once, not per checkpoint")
307+
})
308+
309+
t.Run("remote absence not memoized", func(t *testing.T) {
310+
t.Parallel()
311+
store := newRefsStore(t)
312+
calls := 0
313+
store.SetRefFetcher(func(_ context.Context, rn plumbing.ReferenceName) error {
314+
calls++
315+
return fmt.Errorf("ref %s not on remote: %w", rn, plumbing.ErrReferenceNotFound)
316+
})
317+
318+
for _, cid := range []string{"aaaaaaaaaaaa", "bbbbbbbbbbbb"} {
319+
err := store.Write(ctx, SessionSummary{
320+
CheckpointID: id.MustCheckpointID(cid),
321+
Summary: &Summary{Intent: "x"},
322+
})
323+
require.ErrorIs(t, err, ErrCheckpointNotFound)
324+
}
325+
assert.Equal(t, 2, calls, "absence is per-ref and must not suppress later fetches")
326+
})
327+
}
328+
255329
// TestGitRefsStore_BackfillAbsentAfterFetchIsNotFound pins the genuine-absence
256330
// contract: a fetch that succeeds but restores no ref means the checkpoint
257331
// really does not exist in this backend, and the backfill reports the
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
package remote
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"fmt"
7+
"time"
8+
9+
"github.com/go-git/go-git/v6/plumbing"
10+
)
11+
12+
// WriteProbeFetchBudget bounds the on-demand ref fetch performed by a
13+
// BACKFILL's absence probe. Write paths run inside git hooks (post-commit,
14+
// stop-time finalize) where a dead network must not stall the user's
15+
// workflow for the read path's full fetch window; combined with the
16+
// per-store failure memo in the git-refs store, a loop over N checkpoints
17+
// pays a dead network once, briefly.
18+
const WriteProbeFetchBudget = 15 * time.Second
19+
20+
// readFetchTimeout bounds the interactive read-path fetch (unchanged from
21+
// the historical FetchCheckpointRef behavior).
22+
const readFetchTimeout = 2 * time.Minute
23+
24+
// CheckpointFetchTarget returns the git remote (URL or name) that checkpoint
25+
// data is fetched from. It prefers the effective URL resolved by FetchURL,
26+
// which is the source of truth for checkpoint fetch location. If URL
27+
// resolution fails, it falls back to the origin remote name so callers can
28+
// still attempt a fetch.
29+
func CheckpointFetchTarget(ctx context.Context) string {
30+
url, err := FetchURL(ctx)
31+
if err == nil && url != "" {
32+
return url
33+
}
34+
return "origin"
35+
}
36+
37+
// FetchCheckpointRef fetches a single per-checkpoint ref
38+
// (refs/entire/checkpoints/<shard>/<id>) from the checkpoint remote into the
39+
// local ref of the same name, so the git-refs store can resolve a checkpoint
40+
// written on another machine.
41+
//
42+
// Contract — absence is distinguishable from failure:
43+
// - The remote genuinely lacking the ref returns an error wrapping
44+
// plumbing.ErrReferenceNotFound (probed via ls-remote before fetching,
45+
// because `git fetch` of a missing refspec fails indistinguishably from a
46+
// transport error). Store probes classify this as "checkpoint not found",
47+
// which write routing may legitimately act on.
48+
// - Any transport-level failure (probe or fetch) is surfaced as a real
49+
// error, never mapped to absence — a false "absent" would misdirect a
50+
// backfill onto another backend instead of retrying.
51+
func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error {
52+
ctx, cancel := context.WithTimeout(ctx, readFetchTimeout)
53+
defer cancel()
54+
55+
fetchTarget := CheckpointFetchTarget(ctx)
56+
57+
out, err := LsRemoteInDir(ctx, "", fetchTarget, ref.String())
58+
if err != nil {
59+
// Redact: fetchTarget can be a remote URL with embedded credentials
60+
// (CI origin URLs), and this error is logged and shown to users.
61+
return fmt.Errorf("probe checkpoint ref %s on %s: %w", ref, RedactURL(fetchTarget), err)
62+
}
63+
if len(bytes.TrimSpace(out)) == 0 {
64+
return fmt.Errorf("checkpoint ref %s not found on %s: %w", ref, RedactURL(fetchTarget), plumbing.ErrReferenceNotFound)
65+
}
66+
67+
refSpec := "+" + ref.String() + ":" + ref.String()
68+
if _, err := Fetch(ctx, FetchOptions{
69+
Remote: fetchTarget,
70+
RefSpecs: []string{refSpec},
71+
NoTags: true,
72+
}); err != nil {
73+
return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, RedactURL(fetchTarget), err)
74+
}
75+
return nil
76+
}
77+
78+
// BoundedCheckpointRefFetcher returns a RefFetchFunc-shaped fetcher whose
79+
// per-call budget is capped at d, for wiring into write-path checkpoint
80+
// stores (see WriteProbeFetchBudget).
81+
func BoundedCheckpointRefFetcher(d time.Duration) func(context.Context, plumbing.ReferenceName) error {
82+
return func(ctx context.Context, ref plumbing.ReferenceName) error {
83+
ctx, cancel := context.WithTimeout(ctx, d)
84+
defer cancel()
85+
return FetchCheckpointRef(ctx, ref)
86+
}
87+
}
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
package remote
2+
3+
import (
4+
"context"
5+
"os/exec"
6+
"testing"
7+
8+
"github.com/go-git/go-git/v6/plumbing"
9+
"github.com/stretchr/testify/require"
10+
11+
"github.com/entireio/cli/cmd/entire/cli/testutil"
12+
)
13+
14+
// checkpointRefFixture creates a work repo whose origin is a local bare repo
15+
// holding (or not holding) a checkpoint ref, and chdirs into the work repo so
16+
// fetch-target resolution finds origin.
17+
func checkpointRefFixture(t *testing.T, withRef bool) (workDir string, ref plumbing.ReferenceName) {
18+
t.Helper()
19+
bareDir := t.TempDir()
20+
out, err := exec.CommandContext(t.Context(), "git", "init", "--bare", bareDir).CombinedOutput()
21+
require.NoError(t, err, "git init --bare: %s", out)
22+
23+
workDir = t.TempDir()
24+
testutil.InitRepo(t, workDir)
25+
testutil.WriteFile(t, workDir, "f.txt", "content")
26+
testutil.GitAdd(t, workDir, "f.txt")
27+
testutil.GitCommit(t, workDir, "init")
28+
out, err = exec.CommandContext(t.Context(), "git", "-C", workDir, "remote", "add", "origin", bareDir).CombinedOutput()
29+
require.NoError(t, err, "git remote add: %s", out)
30+
31+
ref = plumbing.ReferenceName("refs/entire/checkpoints/Z9/01KVBJCWYA4YW6J5M9GP655HZ9")
32+
if withRef {
33+
out, err = exec.CommandContext(t.Context(), "git", "-C", workDir, "push", "--quiet", "origin", "HEAD:"+ref.String()).CombinedOutput()
34+
require.NoError(t, err, "git push checkpoint ref: %s", out)
35+
}
36+
37+
t.Chdir(workDir)
38+
return workDir, ref
39+
}
40+
41+
// TestFetchCheckpointRef_RemoteMissingRefIsAbsence: a remote that does not
42+
// have the requested checkpoint ref is ABSENCE, not a transport failure — the
43+
// error must wrap plumbing.ErrReferenceNotFound so store probes (reads and
44+
// backfill writes) classify it as "checkpoint not found" and, under kind
45+
// routing, may legitimately fall through to another backend. Before this
46+
// distinction, git fetch of a missing refspec failed like a network error,
47+
// which made wiring a fetcher into write paths unsafe.
48+
func TestFetchCheckpointRef_RemoteMissingRefIsAbsence(t *testing.T) {
49+
_, ref := checkpointRefFixture(t, false)
50+
51+
err := FetchCheckpointRef(context.Background(), ref)
52+
require.Error(t, err)
53+
require.ErrorIs(t, err, plumbing.ErrReferenceNotFound,
54+
"a ref the remote does not have must classify as absence")
55+
}
56+
57+
// TestFetchCheckpointRef_PresentRefFetches: the ref exists on the remote but
58+
// not locally; the fetch must create the local ref of the same name.
59+
func TestFetchCheckpointRef_PresentRefFetches(t *testing.T) {
60+
workDir, ref := checkpointRefFixture(t, true)
61+
62+
require.NoError(t, FetchCheckpointRef(context.Background(), ref))
63+
64+
out, err := exec.CommandContext(t.Context(), "git", "-C", workDir, "show-ref", "--verify", ref.String()).CombinedOutput()
65+
require.NoError(t, err, "fetched ref must exist locally: %s", out)
66+
}
67+
68+
// TestFetchCheckpointRef_UnreachableRemoteIsFailure: a transport-level
69+
// failure (unreachable remote) must NOT classify as absence.
70+
func TestFetchCheckpointRef_UnreachableRemoteIsFailure(t *testing.T) {
71+
workDir, ref := checkpointRefFixture(t, false)
72+
out, err := exec.CommandContext(t.Context(), "git", "-C", workDir, "remote", "set-url", "origin", workDir+"/nonexistent-remote").CombinedOutput()
73+
require.NoError(t, err, "%s", out)
74+
75+
err = FetchCheckpointRef(context.Background(), ref)
76+
require.Error(t, err)
77+
require.NotErrorIs(t, err, plumbing.ErrReferenceNotFound,
78+
"a transport failure must stay distinguishable from absence")
79+
}

cmd/entire/cli/explain.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/entireio/cli/cmd/entire/cli/agent/types"
2424
"github.com/entireio/cli/cmd/entire/cli/checkpoint"
2525
"github.com/entireio/cli/cmd/entire/cli/checkpoint/id"
26+
"github.com/entireio/cli/cmd/entire/cli/checkpoint/remote"
2627
"github.com/entireio/cli/cmd/entire/cli/interactive"
2728
"github.com/entireio/cli/cmd/entire/cli/logging"
2829
"github.com/entireio/cli/cmd/entire/cli/palette"
@@ -719,7 +720,12 @@ func runExplainCheckpointWithLookup(ctx context.Context, w, errW io.Writer, chec
719720
return err
720721
}
721722
stopLoad(false) // generation prints its own progress to w/errW
722-
writeStores, openErr := checkpoint.Open(ctx, lookup.repo, checkpoint.OpenOptions{})
723+
// RefFetcher: the summary backfill's absence probe fetches a ref that
724+
// exists remotely but not locally (written/migrated on another
725+
// machine), bounded by the write-probe budget.
726+
writeStores, openErr := checkpoint.Open(ctx, lookup.repo, checkpoint.OpenOptions{
727+
RefFetcher: remote.BoundedCheckpointRefFetcher(remote.WriteProbeFetchBudget),
728+
})
723729
if openErr != nil {
724730
return fmt.Errorf("open checkpoint store: %w", openErr)
725731
}

cmd/entire/cli/git_operations.go

Lines changed: 8 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -462,43 +462,18 @@ func FetchMetadataFromCheckpointRemote(ctx context.Context) error {
462462
}
463463

464464
// resolveCheckpointFetchTarget returns the fetch target for checkpoint data.
465-
// It prefers the effective URL resolved by checkpoint/remote.FetchURL, which is
466-
// the source of truth for checkpoint fetch location. If URL resolution fails, it
467-
// falls back to the origin remote name so callers can still attempt a fetch.
465+
// Thin alias for remote.CheckpointFetchTarget (the single source of truth).
468466
func resolveCheckpointFetchTarget(ctx context.Context) string {
469-
url, err := remote.FetchURL(ctx)
470-
if err == nil && url != "" {
471-
return url
472-
}
473-
return "origin"
467+
return remote.CheckpointFetchTarget(ctx)
474468
}
475469

476-
// FetchCheckpointRef fetches a single per-checkpoint ref (refs/entire/checkpoints/
477-
// <shard>/<id>) from the checkpoint remote into the local ref of the same name,
478-
// so the git-refs store can resolve a checkpoint written on another machine.
479-
//
480-
// Contract: a fetch failure is surfaced as a real error, never mapped to
481-
// "checkpoint not found" — callers (resolveRefMaybeFetch, for both reads and
482-
// backfill writes) deliberately keep transient unavailability distinguishable
483-
// from absence. Note this includes a ref the remote does not have: git fetch
484-
// of an explicit refspec fails outright, so "absent on the remote" currently
485-
// surfaces as a fetch error, not as absence.
470+
// FetchCheckpointRef fetches a single per-checkpoint ref from the checkpoint
471+
// remote. Thin alias for remote.FetchCheckpointRef, kept so existing cli-side
472+
// call sites and OpenOptions wiring stay unchanged; see that function for the
473+
// absence-vs-failure contract (remote-lacks-ref wraps
474+
// plumbing.ErrReferenceNotFound; transport failures surface as-is).
486475
func FetchCheckpointRef(ctx context.Context, ref plumbing.ReferenceName) error {
487-
ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
488-
defer cancel()
489-
490-
fetchTarget := resolveCheckpointFetchTarget(ctx)
491-
refSpec := "+" + ref.String() + ":" + ref.String()
492-
if _, err := remote.Fetch(ctx, remote.FetchOptions{
493-
Remote: fetchTarget,
494-
RefSpecs: []string{refSpec},
495-
NoTags: true,
496-
}); err != nil {
497-
// Redact: fetchTarget can be a remote URL with embedded credentials
498-
// (CI origin URLs), and this error is logged and shown to users.
499-
return fmt.Errorf("fetch checkpoint ref %s from %s: %w", ref, remote.RedactURL(fetchTarget), err)
500-
}
501-
return nil
476+
return remote.FetchCheckpointRef(ctx, ref) //nolint:wrapcheck // thin alias; the remote error carries full context
502477
}
503478

504479
// FetchBlobsByHash fetches specific blob objects from the remote by their SHA-1 hashes.

0 commit comments

Comments
 (0)