migrate: re-enqueue already-imported refs so a lost enqueue still pushes

605050d→main·Soph·1w ago·2 files·+75 added/-16 removed

The idempotency skip keyed only on "snapshot already on the ref," so a ref written by a prior run that then failed to enqueue it — an Enqueue error, or a crash between setRef and Enqueue — was skipped on every later run and never queued for push. The migrated ref then existed locally but its checkpoint data never reached the remote, silently defeating the guaranteed-enqueue contract on exactly the retry it was meant to enable.

Compute the ref name up front and enqueue on the skip path too (never in dry-run). Enqueue is idempotent — duplicates collapse on Drain and an already-pushed ref is a no-op on the next push — so re-enqueuing a skipped checkpoint is safe and closes the gap. Dry-run now accounts an already-imported checkpoint as a skip rather than reaching the old would-migrate branch, and writes/enqueues nothing.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

Sessions

01KWY6VAT1V2Z6NHCKZ3BNNJY2View transcript

Changes

2

• cmd/entire/cli/checkpoint

• Mmigrate.go+37/-16

• Mmigrate_test.go+38

40 unmodified lines
41
42
43
44
45
46
44
45
46
47
48
49
50
25 unmodified lines

76
77
78
79
80
81
82
83
84
85
86
12 unmodified lines

99
100
101
96
97
98
99
100
101
102
103
102
103
104
105
106
107
108
106
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
8 unmodified lines

143
144
145
121
122
123
124
146
147
148

40 unmodified lines

// a re-run after more branch activity fast-forwards the ref (parenting on the
// existing commit).
//
// New and advanced refs are enqueued for push — a failed enqueue is an error,
// not best-effort. This function does not push. When dryRun is true it reports
// what would change without writing refs.
// Refs are enqueued for push — a failed enqueue is an error, not best-effort —
// including already-imported refs, so a ref left unqueued by a partial earlier
// run still gets pushed. This function does not push. When dryRun is true it
// reports what would change without writing or enqueuing anything.
func MigrateBranchToRefs(ctx context.Context, repo *git.Repository, dryRun bool) (MigrateResult, error) {
    var result MigrateResult

25 unmodified lines

return fmt.Errorf("normalize checkpoint %s: %w", cid, err)
    }

refName, err := RefName(cid)
    if err != nil {
        return fmt.Errorf("ref name for checkpoint %s: %w", cid, err)
    }

// The existing ref drives the idempotency check and the new commit's
    // parent. refBase separates three cases we must not conflate:
    //   - no ref yet (nil error, zero hash): a brand-new orphan.
12 unmodified lines

default:
        return fmt.Errorf("resolve existing ref for checkpoint %s: %w", cid, err)
    }
    // Skip when this snapshot was already imported anywhere on the ref's
    // first-parent chain: the ref may have advanced past it through
    // refs-store writes, and re-wrapping the old snapshot would regress
    // the tip.
    if treeInRefHistory(repo, parent, migratedTree) {
        result.Skipped++
        return nil
    }
    // This snapshot is already imported when it appears anywhere on the
    // ref's first-parent chain: the ref may have advanced past it through
    // refs-store writes, and re-wrapping the old snapshot would regress the
    // tip.
    alreadyImported := treeInRefHistory(repo, parent, migratedTree)

if dryRun {
        result.Migrated = append(result.Migrated, cid)
        // Report only what a real run would newly write; an already-imported
        // checkpoint is a skip, not a would-migrate. No refs or objects are
        // enqueued or written on this path.
        if alreadyImported {
            result.Skipped++
        } else {
            result.Migrated = append(result.Migrated, cid)
        }
        return nil
    }

if alreadyImported {
        // The snapshot is on the ref, but a prior run may have written the
        // ref and then failed before enqueuing it (an Enqueue error, or a
        // crash between setRef and Enqueue), leaving it queued for a push
        // that never comes — and every later run would skip it here. Enqueue
        // unconditionally so the "queued for push" contract survives a
        // partial earlier run; duplicates collapse on Drain and an
        // already-pushed ref is a no-op on the next push.
        if err := queue.Enqueue(refName); err != nil {
            return fmt.Errorf("enqueue checkpoint %s for push: %w", cid, err)
        }
        result.Skipped++
        return nil
    }

8 unmodified lines

// setRef's own enqueue is best-effort (a condensation write must not
    // fail on it); the migration's queued-for-push contract needs a
    // guaranteed one. Duplicates collapse on Drain.
    refName, err := RefName(cid)
    if err != nil {
        return fmt.Errorf("ref name for checkpoint %s: %w", cid, err)
    }
    if err := queue.Enqueue(refName); err != nil {
        return fmt.Errorf("enqueue checkpoint %s for push: %w", cid, err)
    }
}`