diff --git a/CHANGELOG.md b/CHANGELOG.md index a89b1770..9c2de745 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/), and this project adheres to [Semantic Versioning](https://semver.org/). +## [Unreleased] + +### Fixed + +- **A batched bootstrap no longer discards its resume position when the target refuses the branch create.** The cutover push carries two commands — advance the `refs/gitsync/bootstrap/heads/` temp ref to the final checkpoint, and create the real branch there — and the temp ref was deleted on the strength of that push returning no error. Under `--best-effort` (implied by `--all-refs`) a per-ref `ng` reaches a callback and the push still returns `nil`, so a target that refuses the create — a protected branch, a pre-receive policy — ended the run with neither the branch nor the marker and reported success. With nothing on the target referencing the objects already delivered, the next run had no fetch have to negotiate against and re-transferred the entire history, on precisely the large repositories batching exists for. Whether the branch landed is now settled against the target's own refs, not inferred from the push, and the marker survives anything short of the branch being present at the hash the run pushed ([#117](https://github.com/entireio/git-sync/pull/117)) + +### Changed + +- **A target that refuses the batched bootstrap temp ref now fails the run, including under `--best-effort`.** Every other per-ref refusal stays a warning there, but this one is the batching state machine: the run previously advanced its checkpoint position against a ref the target had not moved, then failed later and more confusingly, or deleted a marker at a hash the target never accepted. A run against a target that blocks writes to `refs/gitsync/*` used to exit 0 with warnings and now exits non-zero — worth knowing if you alert on exit codes ([#117](https://github.com/entireio/git-sync/pull/117)) + ## [0.9.0] - 2026-08-28 ### Security diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index 2eb0022e..a2d22806 100644 --- a/internal/gitproto/push.go +++ b/internal/gitproto/push.go @@ -50,6 +50,111 @@ type Pusher struct { // uses the env-or-default limit (see MaxRefUpdatesEnv); a positive value // overrides it — e.g. from the --target-max-ref-updates flag. MaxRefUpdates int + + // lastOutcomes holds what the target said about each ref in the most + // recent Push* call on this Pusher, reset when that call starts. A caller + // that infers "the ref landed" from a nil error needs THAT request's + // answer, not a session-wide accumulation: the same ref name can be + // pushed more than once in a run. + lastOutcomes map[plumbing.ReferenceName]refAnswer +} + +// refAnswer is one ref's entry in a decoded report-status. +type refAnswer struct { + refused bool + reason string +} + +// RefOutcome is what a target did with one ref command in a push. +type RefOutcome int + +const ( + // RefOutcomeUnknown means the target said nothing about this ref: it did + // not negotiate report-status, its report omitted this command, or no push + // has carried the ref. Distinct from acceptance — silence is not something + // the target authored — and distinct from refusal. + RefOutcomeUnknown RefOutcome = iota + // RefOutcomeApplied means the target reported "ok" for this ref. + RefOutcomeApplied + // RefOutcomeRefused means the target reported "ng" for this ref. Under + // BestEffort that is survivable and the push still returns nil, which is + // why asking is necessary at all. + RefOutcomeRefused +) + +// LastOutcome reports what the target did with name in the most recent Push* +// call on this Pusher, and the target's "ng" reason when it refused. Answers +// by value rather than handing out the Pusher's map, so a caller cannot mutate +// or race with push state it does not own. +func (p *Pusher) LastOutcome(name plumbing.ReferenceName) (RefOutcome, string) { + answer, seen := p.lastOutcomes[name] + switch { + case seen && answer.refused: + return RefOutcomeRefused, answer.reason + case seen: + return RefOutcomeApplied, "" + default: + return RefOutcomeUnknown, "" + } +} + +// pushStatusSink observes the target's per-ref answers for one request. A nil +// Rejected is what makes a per-ref "ng" fatal, so it stays the switch between +// best-effort and strict — recording outcomes must not quietly turn every push +// best-effort. +type pushStatusSink struct { + Rejected func(plumbing.ReferenceName, string) + Applied func(plumbing.ReferenceName) +} + +// fatalRejections reports whether a per-ref "ng" must fail the push. +func (s *pushStatusSink) fatalRejections() bool { + return s == nil || s.Rejected == nil +} + +// record files every per-ref status in a decoded report. +func (s *pushStatusSink) record(report *packp.ReportStatus) { + if s == nil { + return + } + for _, cs := range report.CommandStatuses { + if cs.Status == "" || cs.Status == "ok" { + if s.Applied != nil { + s.Applied(cs.ReferenceName) + } + continue + } + if s.Rejected != nil { + s.Rejected(cs.ReferenceName, cs.Status) + } + } +} + +// beginPush clears the previous push's outcomes and returns the sink for this +// one. Applied is always recorded, so a strict caller — whose nil error really +// does prove every command landed — can be told so without a second round +// trip; Rejected is installed only when the caller opted into best-effort. +func (p *Pusher) beginPush() *pushStatusSink { + p.lastOutcomes = nil + sink := &pushStatusSink{ + Applied: func(name plumbing.ReferenceName) { + p.answer(name, refAnswer{}) + }, + } + if p.OnRejection != nil { + sink.Rejected = func(name plumbing.ReferenceName, status string) { + p.answer(name, refAnswer{refused: true, reason: status}) + p.OnRejection(name, status) + } + } + return sink +} + +func (p *Pusher) answer(name plumbing.ReferenceName, answer refAnswer) { + if p.lastOutcomes == nil { + p.lastOutcomes = make(map[plumbing.ReferenceName]refAnswer, 1) + } + p.lastOutcomes[name] = answer } // NewPusher builds a target-side push executor. @@ -144,18 +249,18 @@ func logRefUpdateBatch(conn Conn, verbose bool, batchNum, totalBatches, refs int // PushPack streams a pack to the target. func (p *Pusher) PushPack(ctx context.Context, commands []PushCommand, pack io.ReadCloser) error { - return PushPack(ctx, p.Conn, p.Adv, commands, pack, p.MaxRefUpdates, p.Verbose, p.OnRejection) + return pushPack(ctx, p.Conn, p.Adv, commands, pack, p.MaxRefUpdates, p.Verbose, p.beginPush()) } // PushCommands sends ref-only updates. Creates/updates carry an empty pack; -// delete-only pushes carry no pack. See the package-level PushCommands. +// delete-only pushes carry no pack. See the package-level pushCommands. func (p *Pusher) PushCommands(ctx context.Context, commands []PushCommand) error { - return PushCommands(ctx, p.Conn, p.Adv, commands, p.MaxRefUpdates, p.Verbose, p.OnRejection) + return pushCommands(ctx, p.Conn, p.Adv, commands, p.MaxRefUpdates, p.Verbose, p.beginPush()) } // PushObjects encodes and pushes locally materialized objects. func (p *Pusher) PushObjects(ctx context.Context, commands []PushCommand, store storer.Storer, hashes []plumbing.Hash) error { - return PushObjects(ctx, p.Conn, p.Adv, commands, store, hashes, p.MaxRefUpdates, p.Verbose, p.OnRejection) + return pushObjects(ctx, p.Conn, p.Adv, commands, store, hashes, p.MaxRefUpdates, p.Verbose, p.beginPush()) } // buildUpdateRequest builds the receive-pack update request. @@ -408,7 +513,7 @@ func sendReceivePack( req *packp.UpdateRequests, packData io.Reader, verbose bool, - onRejection func(plumbing.ReferenceName, string), + statuses *pushStatusSink, ) error { var header bytes.Buffer if err := req.Encode(&header); err != nil { @@ -450,29 +555,32 @@ func sendReceivePack( if err := report.Decode(respReader); err != nil { return fmt.Errorf("decode report-status: %w", err) } - if onRejection == nil { + if statuses.fatalRejections() { if err := report.Error(); err != nil { return fmt.Errorf("report-status: %w", asRefRejectedError(annotateLeaseFailure(err))) } + // Nothing was refused and refusals are fatal, so this nil really + // does mean the target applied every command it reported. + statuses.record(report) return nil } if report.UnpackStatus != "" && report.UnpackStatus != "ok" { // Server-authored, and formatted directly rather than wrapped, so - // it needs filtering here — the sibling branch below gets it from + // it needs filtering here — the sibling branch above gets it from // sanitizedError. return fmt.Errorf("report-status: unpack error: %s", sanitize.Text(report.UnpackStatus)) } - for _, cs := range report.CommandStatuses { - if cs.Status == "" || cs.Status == "ok" { - continue - } - onRejection(cs.ReferenceName, cs.Status) - } + // A conforming receive-pack reports one status per command, so a + // command this report never mentions stays RefOutcomeUnknown. That is + // deliberately not a refusal: nothing was refused, so nothing should + // fail, but the silence is also not the target confirming the ref, and + // a caller that has to know can go and look. + statuses.record(report) } return nil } -// PushObjects pushes locally-materialized objects to the target. +// pushObjects pushes locally-materialized objects to the target. // // A push within the per-request ref-update limit (see effectiveMaxRefUpdates) // is a single atomic receive-pack request. A larger push is split: the @@ -480,7 +588,7 @@ func sendReceivePack( // with the first batch of object-bearing commands, then the remaining refs (and // any deletes) move as ref-only updates because the objects are already // committed. -func PushObjects( +func pushObjects( ctx context.Context, conn Conn, adv *packp.AdvRefs, @@ -489,11 +597,11 @@ func PushObjects( hashes []plumbing.Hash, maxRefUpdates int, verbose bool, - onRejection func(plumbing.ReferenceName, string), + statuses *pushStatusSink, ) error { limit := effectiveMaxRefUpdates(maxRefUpdates) if len(commands) <= limit { - return pushObjectsBatch(ctx, conn, adv, commands, store, hashes, verbose, onRejection) + return pushObjectsBatch(ctx, conn, adv, commands, store, hashes, verbose, statuses) } updates := make([]PushCommand, 0, len(commands)) @@ -508,17 +616,17 @@ func PushObjects( if len(updates) > 0 { first, rest := splitFirstBatch(updates, limit) - if err := pushObjectsBatch(ctx, conn, adv, first, store, hashes, verbose, onRejection); err != nil { + if err := pushObjectsBatch(ctx, conn, adv, first, store, hashes, verbose, statuses); err != nil { return err } if len(rest) > 0 { - if err := PushCommands(ctx, conn, adv, rest, maxRefUpdates, verbose, onRejection); err != nil { + if err := pushCommands(ctx, conn, adv, rest, maxRefUpdates, verbose, statuses); err != nil { return err } } } if len(deletes) > 0 { - return PushCommands(ctx, conn, adv, deletes, maxRefUpdates, verbose, onRejection) + return pushCommands(ctx, conn, adv, deletes, maxRefUpdates, verbose, statuses) } return nil } @@ -542,14 +650,14 @@ func pushObjectsBatch( store storer.Storer, hashes []plumbing.Hash, verbose bool, - onRejection func(plumbing.ReferenceName, string), + statuses *pushStatusSink, ) error { req, _, hasUpdates, err := buildUpdateRequest(adv, commands, verbose) if err != nil { return err } if !hasUpdates { - return sendReceivePack(ctx, conn, req, nil, verbose, onRejection) + return sendReceivePack(ctx, conn, req, nil, verbose, statuses) } progressDest := progressSink(verbose, "target: ", conn.ProgressWriter()) @@ -577,7 +685,7 @@ func pushObjectsBatch( done <- pw.Close() }() - err = sendReceivePack(ctx, conn, req, pr, verbose, onRejection) + err = sendReceivePack(ctx, conn, req, pr, verbose, statuses) _ = pr.Close() encodeErr := <-done if err != nil { @@ -590,7 +698,7 @@ func pushObjectsBatch( // fixed []*packfile.ObjectToPack, ignoring its arguments. It is the // passthrough used by PushObjects to feed pre-selected objects back // into packfile.Encoder via WithObjectSelector. Used exactly once per -// PushObjects call and not exposed outside this package. +// pushObjects call and not exposed outside this package. type precomputedSelector struct { objects []*packfile.ObjectToPack } @@ -620,7 +728,7 @@ func (cw *countingWriter) Count() int64 { return cw.n.Load() } // startSelectionProgress emits in-place "selecting deltas, elapsed X" // updates every 500ms during the synchronous delta-selection phase of -// PushObjects. The returned stop function takes the number of selected +// pushObjects. The returned stop function takes the number of selected // objects and the selection error (nil on success); on success it // finalizes the line with a permanent "selected N objects in Y" // summary, on error it just stops the ticker without claiming success. @@ -725,8 +833,8 @@ func HumanBytes(n int64) string { return fmt.Sprintf("%.2f %s", value, suffix) } -// PushPack pushes a pack stream (relay) to the target. -func PushPack( +// pushPack pushes a pack stream (relay) to the target. +func pushPack( ctx context.Context, conn Conn, adv *packp.AdvRefs, @@ -734,7 +842,7 @@ func PushPack( pack io.ReadCloser, maxRefUpdates int, verbose bool, - onRejection func(plumbing.ReferenceName, string), + statuses *pushStatusSink, ) error { for _, cmd := range commands { if cmd.Delete { @@ -755,7 +863,7 @@ func PushPack( return err } - err = sendReceivePack(ctx, conn, req, pack, verbose, onRejection) + err = sendReceivePack(ctx, conn, req, pack, verbose, statuses) closeErr := pack.Close() if err != nil { return err @@ -765,12 +873,12 @@ func PushPack( } if len(rest) > 0 { - return PushCommands(ctx, conn, adv, rest, maxRefUpdates, verbose, onRejection) + return pushCommands(ctx, conn, adv, rest, maxRefUpdates, verbose, statuses) } return nil } -// PushCommands sends ref update commands that move no new objects to the +// pushCommands sends ref update commands that move no new objects to the // target — the referenced objects already exist there. // // A create/update command still carries a valid empty pack (12-byte header, @@ -780,21 +888,21 @@ func PushPack( // commands; an explicit empty pack satisfies them and stays valid for servers // that tolerate the pack-less form. Delete-only pushes carry no pack, as git // requires. -func PushCommands( +func pushCommands( ctx context.Context, conn Conn, adv *packp.AdvRefs, commands []PushCommand, maxRefUpdates int, verbose bool, - onRejection func(plumbing.ReferenceName, string), + statuses *pushStatusSink, ) error { batches := chunkRefUpdates(commands, effectiveMaxRefUpdates(maxRefUpdates)) for i, batch := range batches { // Ref-only batches carry no useful target progress; suppress the empty // sideband (verbose=false) and report completion ourselves so a large // push doesn't spew a bare "target:" line per batch. - if err := pushCommandsBatch(ctx, conn, adv, batch, false, onRejection); err != nil { + if err := pushCommandsBatch(ctx, conn, adv, batch, false, statuses); err != nil { return err } logRefUpdateBatch(conn, verbose, i+1, len(batches), len(batch)) @@ -810,7 +918,7 @@ func pushCommandsBatch( adv *packp.AdvRefs, commands []PushCommand, verbose bool, - onRejection func(plumbing.ReferenceName, string), + statuses *pushStatusSink, ) error { req, _, hasUpdates, err := buildUpdateRequest(adv, commands, verbose) if err != nil { @@ -820,7 +928,7 @@ func pushCommandsBatch( if hasUpdates { packData = bytes.NewReader(emptyPack(adv)) } - return sendReceivePack(ctx, conn, req, packData, verbose, onRejection) + return sendReceivePack(ctx, conn, req, packData, verbose, statuses) } // emptyPackHeader is the fixed 12-byte prefix of any packfile with zero diff --git a/internal/gitproto/push_test.go b/internal/gitproto/push_test.go index 63679860..80580258 100644 --- a/internal/gitproto/push_test.go +++ b/internal/gitproto/push_test.go @@ -22,6 +22,8 @@ import ( "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-git/go-git/v6/storage/memory" "github.com/stretchr/testify/require" + + "entire.io/entire/git-sync/internal/syncertest" ) func TestPrefixedLineWriter(t *testing.T) { @@ -150,7 +152,7 @@ func TestPushPackClosesPackOnSuccess(t *testing.T) { conn := connForServer(t, srv) adv := &packp.AdvRefs{} - err := PushPack(context.Background(), conn, adv, []PushCommand{{ + err := pushPack(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), }}, pack, 0, false, nil) @@ -177,7 +179,7 @@ func TestPushPackClosesPackOnReceivePackError(t *testing.T) { conn := connForServer(t, srv) adv := &packp.AdvRefs{} - err := PushPack(context.Background(), conn, adv, []PushCommand{{ + err := pushPack(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), }}, pack, 0, false, nil) @@ -207,7 +209,7 @@ func TestPushPackClosesPackOnContextCanceled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) done := make(chan error, 1) go func() { - done <- PushPack(ctx, conn, adv, []PushCommand{{ + done <- pushPack(ctx, conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), }}, pack, 0, false, nil) @@ -263,7 +265,7 @@ func TestPushPackStartsHTTPBeforePackFullyRead(t *testing.T) { done := make(chan error, 1) go func() { - done <- PushPack(context.Background(), conn, adv, []PushCommand{{ + done <- pushPack(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), }}, pack, 0, false, nil) @@ -320,7 +322,7 @@ func TestPushObjectsStreamsBody(t *testing.T) { adv := &packp.AdvRefs{} adv.Capabilities.Set(capability.OFSDelta) - err := PushObjects(context.Background(), conn, adv, []PushCommand{{ + err := pushObjects(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/main", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), }}, memory.NewStorage(), nil, 0, false, nil) @@ -416,7 +418,7 @@ func TestPushCommandsSendsEmptyPackForCreate(t *testing.T) { conn := connForServer(t, srv) adv := &packp.AdvRefs{} - err := PushCommands(context.Background(), conn, adv, []PushCommand{{ + err := pushCommands(context.Background(), conn, adv, []PushCommand{{ Name: "refs/heads/docs-rules", New: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), }}, 0, false, nil) @@ -434,7 +436,7 @@ func TestPushCommandsSendsNoPackForDeleteOnly(t *testing.T) { adv := &packp.AdvRefs{} adv.Capabilities.Set(capability.DeleteRefs) - err := PushCommands(context.Background(), conn, adv, []PushCommand{{ + err := pushCommands(context.Background(), conn, adv, []PushCommand{{ Name: "refs/gitsync/bootstrap/heads/docs-rules", Old: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), Delete: true, @@ -493,7 +495,7 @@ func TestPushPackRejectsDeletes(t *testing.T) { require.NoError(t, err) conn := &HTTPConn{EndpointURL: ep, HTTP: &http.Client{}} - err = PushPack(context.Background(), conn, adv, []PushCommand{ + err = pushPack(context.Background(), conn, adv, []PushCommand{ {Name: "refs/heads/old", Delete: true}, }, pack, 0, false, nil) if err == nil { @@ -833,7 +835,7 @@ func TestPushCommandsBatchesOverCap(t *testing.T) { adv := &packp.AdvRefs{} // limit=3, 7 refs → batches of 3, 3, 1. - require.NoError(t, PushCommands(context.Background(), conn, adv, makeCreateCommands(7), 3, false, nil)) + require.NoError(t, pushCommands(context.Background(), conn, adv, makeCreateCommands(7), 3, false, nil)) rec.mu.Lock() defer rec.mu.Unlock() @@ -860,7 +862,7 @@ func TestPushPackBatchesOverCap(t *testing.T) { pack := io.NopCloser(bytes.NewReader(marker)) // limit=3, 7 refs → first batch of 3 carries the pack, then 3 + 1 ref-only. - require.NoError(t, PushPack(context.Background(), conn, adv, makeCreateCommands(7), pack, 3, false, nil)) + require.NoError(t, pushPack(context.Background(), conn, adv, makeCreateCommands(7), pack, 3, false, nil)) rec.mu.Lock() defer rec.mu.Unlock() @@ -889,7 +891,7 @@ func TestPushCommandsVerboseLogsBatches(t *testing.T) { var buf bytes.Buffer conn := connForServer(t, srv) conn.ProgressOut = &buf - require.NoError(t, PushCommands(context.Background(), conn, adv, makeCreateCommands(7), 3, true, nil)) + require.NoError(t, pushCommands(context.Background(), conn, adv, makeCreateCommands(7), 3, true, nil)) out := buf.String() require.Contains(t, out, "pushed ref-update batch 1/3 (3 refs)") @@ -900,7 +902,7 @@ func TestPushCommandsVerboseLogsBatches(t *testing.T) { var single bytes.Buffer conn2 := connForServer(t, srv) conn2.ProgressOut = &single - require.NoError(t, PushCommands(context.Background(), conn2, adv, makeCreateCommands(2), 3, true, nil)) + require.NoError(t, pushCommands(context.Background(), conn2, adv, makeCreateCommands(2), 3, true, nil)) require.NotContains(t, single.String(), "pushed ref-update batch") } @@ -915,7 +917,7 @@ func TestPushPackUsesDefaultLimitWhenZero(t *testing.T) { adv := &packp.AdvRefs{} pack := io.NopCloser(bytes.NewReader([]byte("PACK-PAYLOAD"))) - require.NoError(t, PushPack(context.Background(), conn, adv, makeCreateCommands(3), pack, 0, false, nil)) + require.NoError(t, pushPack(context.Background(), conn, adv, makeCreateCommands(3), pack, 0, false, nil)) rec.mu.Lock() defer rec.mu.Unlock() @@ -1005,3 +1007,137 @@ func TestPushPackSanitizesUnpackStatusOnBestEffortPath(t *testing.T) { t.Errorf("the real status must survive filtering: %q", err.Error()) } } + +// refReportServer answers every receive-pack POST by denying the named ref +// while reporting every other command "ok". deny is read under mu because the +// handler runs on the server's goroutine while the test drives the pushes. +func refReportServer(t *testing.T, mu *sync.Mutex, deny *plumbing.ReferenceName, reason string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Logf("read request body: %v", err) + } + _ = r.Body.Close() + req := &packp.UpdateRequests{} + if err := req.Decode(bytes.NewReader(body)); err != nil { + t.Logf("decode update requests: %v", err) + } + mu.Lock() + denied := *deny + mu.Unlock() + report := syncertest.DenyRefsReport(req, reason, denied) + w.Header().Set("Content-Type", "application/x-git-receive-pack-result") + w.WriteHeader(http.StatusOK) + if err := report.Encode(w); err != nil { + t.Logf("encode report: %v", err) + } + })) +} + +func TestPusherLastRejectionsCoversOnlyTheLastPush(t *testing.T) { + main := plumbing.ReferenceName("refs/heads/main") + const reason = "deny creating a protected branch" + var mu sync.Mutex + deny := main + srv := refReportServer(t, &mu, &deny, reason) + defer srv.Close() + + adv := &packp.AdvRefs{} + adv.Capabilities.Set(capability.ReportStatus) + pusher := NewPusher(connForServer(t, srv), adv, false) + var session []plumbing.ReferenceName + // Non-nil OnRejection selects the best-effort branch, where a per-ref ng + // reaches the callback and the push returns nil. + pusher.OnRejection = func(name plumbing.ReferenceName, _ string) { + session = append(session, name) + } + + cmds := []PushCommand{{Name: main, Old: plumbing.ZeroHash, New: plumbing.NewHash("1111111111111111111111111111111111111111")}} + if err := pusher.PushCommands(t.Context(), cmds); err != nil { + t.Fatalf("best-effort push returned an error: %v", err) + } + if outcome, got := pusher.LastOutcome(main); outcome != RefOutcomeRefused || got != reason { + t.Fatalf("LastOutcome(%s) = (%v, %q), want (refused, the target's reason)", main, outcome, got) + } + + // Same ref, same Pusher, this time accepted. A session-wide view would + // still call it rejected — the trap this exists to avoid. + mu.Lock() + deny = plumbing.ReferenceName("refs/heads/other") + mu.Unlock() + if err := pusher.PushCommands(t.Context(), cmds); err != nil { + t.Fatalf("second push returned an error: %v", err) + } + if outcome, _ := pusher.LastOutcome(main); outcome != RefOutcomeApplied { + t.Errorf("LastOutcome(%s) = %v after a clean push, want applied", main, outcome) + } + if len(session) != 1 { + t.Errorf("OnRejection called %d times, want 1 — the session view must be unaffected", len(session)) + } +} + +func TestPusherWithoutOnRejectionKeepsRejectionsFatal(t *testing.T) { + main := plumbing.ReferenceName("refs/heads/main") + var mu sync.Mutex + deny := main + srv := refReportServer(t, &mu, &deny, "deny creating a protected branch") + defer srv.Close() + + adv := &packp.AdvRefs{} + adv.Capabilities.Set(capability.ReportStatus) + pusher := NewPusher(connForServer(t, srv), adv, false) + + err := pusher.PushCommands(t.Context(), []PushCommand{{ + Name: main, Old: plumbing.ZeroHash, New: plumbing.NewHash("1111111111111111111111111111111111111111"), + }}) + if err == nil { + t.Fatal("a per-ref ng must stay fatal when the caller installed no OnRejection") + } + if outcome, _ := pusher.LastOutcome(main); outcome != RefOutcomeUnknown { + t.Errorf("LastOutcome(%s) = %v after a failed strict push, want unknown", main, outcome) + } +} + +func TestPusherOutcomeUnknownWhenReportOmitsRef(t *testing.T) { + main := plumbing.ReferenceName("refs/heads/main") + other := plumbing.ReferenceName("refs/heads/other") + // A report that mentions only one of the two commands. A conforming + // receive-pack does not do this; the point is that the ref it skipped must + // not read as accepted, and must not fail the push either. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if _, err := io.Copy(io.Discard, r.Body); err != nil { + t.Logf("drain request body: %v", err) + } + _ = r.Body.Close() + report := &packp.ReportStatus{ + UnpackStatus: "ok", + CommandStatuses: []*packp.CommandStatus{{ReferenceName: main, Status: "ok"}}, + } + w.Header().Set("Content-Type", "application/x-git-receive-pack-result") + w.WriteHeader(http.StatusOK) + if err := report.Encode(w); err != nil { + t.Logf("encode report: %v", err) + } + })) + defer srv.Close() + + adv := &packp.AdvRefs{} + adv.Capabilities.Set(capability.ReportStatus) + pusher := NewPusher(connForServer(t, srv), adv, false) + pusher.OnRejection = func(plumbing.ReferenceName, string) {} + + hash := plumbing.NewHash("1111111111111111111111111111111111111111") + if err := pusher.PushCommands(t.Context(), []PushCommand{ + {Name: main, Old: plumbing.ZeroHash, New: hash}, + {Name: other, Old: plumbing.ZeroHash, New: hash}, + }); err != nil { + t.Fatalf("an omitted status must not fail the push: %v", err) + } + if outcome, _ := pusher.LastOutcome(main); outcome != RefOutcomeApplied { + t.Errorf("LastOutcome(%s) = %v, want applied", main, outcome) + } + if outcome, reason := pusher.LastOutcome(other); outcome != RefOutcomeUnknown { + t.Errorf("LastOutcome(%s) = (%v, %q), want unknown: the target never mentioned it", other, outcome, reason) + } +} diff --git a/internal/strategy/bootstrap/bootstrap.go b/internal/strategy/bootstrap/bootstrap.go index 1c83f0e5..5ecda464 100644 --- a/internal/strategy/bootstrap/bootstrap.go +++ b/internal/strategy/bootstrap/bootstrap.go @@ -89,6 +89,29 @@ type Params struct { // subdivision, switching to batched mode). Implementations should // treat each call as one log line. OnNotice func(string) + // RefOutcome reports what the target did with a ref in the push that just + // returned: applied it, refused it (with the reason), or said nothing about + // it at all. Consulted immediately after a push, since the answer describes + // that one request. + // + // It exists because a nil error is not proof. When the caller pushes + // best-effort the pusher hands a per-ref "ng" to its OnRejection callback + // and returns nil, so a target that refuses one command out of a + // multi-command request reports the whole push as a success. Silence is a + // third answer, not a synonym for success: a target that never negotiated + // report-status, or whose report skipped this command, has confirmed + // nothing. + // + // Nil for a caller that cannot answer, which reads as silence throughout. + RefOutcome func(plumbing.ReferenceName) (gitproto.RefOutcome, string) + // TargetRefsNow re-reads the target's refs. Consulted only when a push + // left the fate of a branch create in doubt — the target refused it, or + // cannot report per-ref status at all — to settle whether the branch is + // there before its resume marker is deleted. That question has one honest + // answer, and it is the target's rather than an inference from the ng text + // it happened to write. Nil or failing means unsettled, which keeps the + // marker. + TargetRefsNow func(context.Context) (map[plumbing.ReferenceName]plumbing.Hash, error) } func (p Params) notice(msg string) { @@ -97,6 +120,56 @@ func (p Params) notice(msg string) { } } +// refOutcome reports what the target did with name in the push that just +// returned. Silence when the caller supplied no accessor. +func (p Params) refOutcome(name plumbing.ReferenceName) (gitproto.RefOutcome, string) { + if p.RefOutcome == nil { + return gitproto.RefOutcomeUnknown, "" + } + return p.RefOutcome(name) +} + +// refRefused reports whether the target refused an update to name, and its +// reason. A refusal is a positive fact — never inferred from silence — because +// it is the one answer this strategy stops a run on. +func (p Params) refRefused(name plumbing.ReferenceName) (string, bool) { + outcome, reason := p.refOutcome(name) + return reason, outcome == gitproto.RefOutcomeRefused +} + +// createConfirmed reports whether the push that just returned proves the branch +// create landed, and what leaves it in doubt when it does not. Doubt is not +// failure: it is settled against the target's own ref listing, because whether +// a branch is there is a fact the target can state and an ng reason's wording +// is not. +func (p Params) createConfirmed(name plumbing.ReferenceName) (bool, string) { + switch outcome, reason := p.refOutcome(name); outcome { + case gitproto.RefOutcomeApplied: + return true, "" + case gitproto.RefOutcomeRefused: + return false, reason + case gitproto.RefOutcomeUnknown: + return false, "target reported no status for this ref" + default: + return false, "target reported no status for this ref" + } +} + +// pendingMarker is a resume marker whose branch create this run could not +// confirm from the push, held back until the target's own ref listing settles +// whether the branch is there. +type pendingMarker struct { + branch plumbing.ReferenceName + tempRef plumbing.ReferenceName + resumeHash plumbing.Hash + // createHash is the hash this run tried to create the branch at. The + // marker is only scaffolding once the branch is there at THAT hash; + // a branch someone else created at an older commit leaves the span + // between the two reachable from nothing but the marker. + createHash plumbing.Hash + doubt string +} + // Result holds the outcome of the bootstrap strategy. Pushed is the count // of attempted ref creates; under BestEffort, callers that want a count // excluding rejected refs need to consult Pusher.OnRejection or apply the @@ -318,6 +391,9 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran // multi-branch bootstrap re-sends all shared objects (e.g., linux's // master and nocache-cleanup share ~99% of history). completedRefs := planner.CopyRefHashMap(p.TargetRefs) + // Markers whose branch create this run could not confirm, settled against + // the target's own ref listing once every branch has had its cutover. + var pending []pendingMarker // calibratedBytesPerObject tracks the per-object byte estimate // updated from observed rejected pushes. Starts at the static @@ -347,6 +423,21 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran if err := p.TargetPusher.PushCommands(ctx, cmds); err != nil { return result, fmt.Errorf("create subsumed branch ref for %s: %w", batch.Plan.TargetRef, err) } + // Same rule as the cutover below — only a create the target + // confirmed counts as delivered — but settled here rather than + // against a ref listing. A subsumed branch has no temp ref to + // preserve, since trunk already delivered its objects, so nothing + // hinges on the answer except the reported batch count and a have + // that trunk's own tip already covers. Neither is worth a round + // trip, and under-reporting work this run cannot confirm is the + // same direction as everything else here. + if confirmed, doubt := p.createConfirmed(batch.Plan.TargetRef); !confirmed { + p.log("bootstrap batch subsumed branch create unconfirmed; not counted", + "branch", batch.Plan.TargetRef.String(), + "source_hash", planner.ShortHash(batch.Plan.SourceHash), + "reason", doubt) + continue + } completedRefs[batch.Plan.TargetRef] = batch.Plan.SourceHash result.BatchCount++ p.log("bootstrap batch subsumed branch finalized", @@ -363,6 +454,13 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran "resume_hash", planner.ShortHash(batch.ResumeHash)) current := batch.ResumeHash + // Whether this branch's create rode a push in this iteration at all. + // It does not when the branch and its marker already sit at the source + // hash — adjustedBootstrapTargetRefs zeroes such a branch so it gets a + // create plan, but the checkpoint loop and the at-tip cutover both have + // nothing to do. Asking what the target said about a ref no push + // carried would read the previous branch's answer. + createPushed := false startIdx, err := planner.BootstrapResumeIndex(batch.Checkpoints, batch.ResumeHash) if err != nil && !batch.ResumeHash.IsZero() && len(batch.chain) > 0 { // Temp ref doesn't match any planned checkpoint (e.g., the user @@ -394,6 +492,16 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran if delErr := p.TargetPusher.PushCommands(ctx, delCmds); delErr != nil { return result, fmt.Errorf("delete stale temp ref %s: %w (original: %w)", batch.TempRef, delErr, err) } + // Restarting from zero is only sound if the marker really went + // away. A refused delete (deletes blocked in the namespace, or the + // ref moved so Old no longer matches) returns nil under + // BestEffort, and continuing would push the first checkpoint with + // Old=zero against a ref that still exists — a CAS failure every + // run reproduces, with the marker never cleared. + if reason, refused := p.refRefused(batch.TempRef); refused { + return result, fmt.Errorf("clear stale bootstrap temp ref %s: target refused: %s (stale because: %w)", + batch.TempRef, reason, err) + } current = plumbing.ZeroHash startIdx = 0 } @@ -439,6 +547,7 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran Reason: fmt.Sprintf("%s -> %s via %s", planner.ShortHash(current), planner.ShortHash(checkpoint), batch.TempRef), }} if idx == len(batch.Checkpoints)-1 { + createPushed = true stagePlans = append(stagePlans, planner.BranchPlan{ Branch: batch.Plan.Branch, SourceRef: batch.Plan.SourceRef, TargetRef: batch.Plan.TargetRef, SourceHash: checkpoint, @@ -625,6 +734,24 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran return result, fmt.Errorf("push bootstrap batch for %s: %w", batch.Plan.TargetRef, pushErr) } _ = packReader.Close() + // The temp ref is the state machine: `current` advances only if + // the target really moved it. A policy that refuses the whole + // request (read-only repository, a blanket pre-receive block) ng's + // this command too, and best-effort turns that into a nil error — + // after which every subsequent checkpoint would negotiate against + // a have the target does not have, and the cutover would delete a + // ref at a hash the target never accepted. There is no partial + // progress to keep here, so stop rather than warn: the branch + // cannot be delivered by this run at all. + // + // Only a refusal stops the run. A target that cannot report + // per-ref status says nothing about this ref either, and treating + // that silence as a refusal would fail every batched bootstrap + // against it. + if reason, refused := p.refRefused(batch.TempRef); refused { + return result, fmt.Errorf("push bootstrap batch for %s: target refused %s: %s", + batch.Plan.TargetRef, batch.TempRef, reason) + } p.log("bootstrap batch checkpoint complete", "branch", batch.Plan.TargetRef.String(), "batch", idx+1, @@ -664,27 +791,90 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran if err := p.TargetPusher.PushCommands(ctx, cmds); err != nil { return result, fmt.Errorf("resume bootstrap cutover for %s: %w", batch.Plan.TargetRef, err) } + createPushed = true + } + + // The temp ref is this import's only record of how far it got, so it + // may only be deleted once the branch it was scaffolding for exists. + // The create rode the final checkpoint's push (or, on an at-tip + // resume, the command push just above), and a nil error from either + // does not prove it landed — see Params.RefOutcome. A target that + // refuses the create (protected branch, pre-receive policy) would + // otherwise end the run with neither the branch nor the marker, + // reported as a success — and with nothing on the target pointing at + // the objects pushed so far, the next run has no have to negotiate + // against and re-transfers the entire history, on precisely the large + // repositories batching exists for. + // + // Unlike the one-shot path, this cannot defer to prune as its + // cleaner: Bootstrap() rejects --prune outright, so on that route no + // cleaner would ever exist and the marker would be permanent. Hence + // the conditional delete rather than no delete. + // A branch nothing was pushed for is one the target already holds at + // the source hash — that is the only way to reach here without a + // create — so its marker is stale and needs no confirming. + confirmed, doubt := true, "" + if createPushed { + confirmed, doubt = p.createConfirmed(batch.Plan.TargetRef) + } + if !confirmed { + // Held back rather than decided here: whether the branch is on the + // target is a fact the target can state, and one listing after the + // last branch settles every doubtful case at once. The marker + // genuinely holds `current` meanwhile, so the objects behind it + // stay usable as haves for the branches planned after this one. + pending = append(pending, pendingMarker{ + branch: batch.Plan.TargetRef, tempRef: batch.TempRef, + resumeHash: current, createHash: batch.Plan.SourceHash, doubt: doubt, + }) + completedRefs[batch.TempRef] = current + p.log("bootstrap batch branch create unconfirmed; deferring marker cleanup", + "branch", batch.Plan.TargetRef.String(), + "temp_ref", batch.TempRef.String(), + "resume_hash", planner.ShortHash(current), + "pushed_checkpoints", len(pushedCheckpoints), + "reason", doubt) + continue } cmds := []gitproto.PushCommand{{Name: batch.TempRef, Old: current, Delete: true}} if err := p.TargetPusher.PushCommands(ctx, cmds); err != nil { return result, fmt.Errorf("delete bootstrap temp ref for %s: %w", batch.Plan.TargetRef, err) } + if reason, refused := p.refRefused(batch.TempRef); refused { + // Not fatal — the branch is there, which is what the operator + // asked for — but the scaffolding outlived it, and on the + // bootstrap route prune is not available to reap it. + p.log("bootstrap temp ref delete refused; marker left on target", + "branch", batch.Plan.TargetRef.String(), + "temp_ref", batch.TempRef.String(), + "reason", reason) + p.notice(fmt.Sprintf("target refused to delete %s (%s) — remove it by hand or let a --prune run reap it", + batch.TempRef, reason)) + } completedRefs[batch.Plan.TargetRef] = batch.Plan.SourceHash completedRefs[batch.TempRef] = batch.Plan.SourceHash p.log("bootstrap batch branch finalized", "branch", batch.Plan.TargetRef.String()) } + // Settled here rather than after the tail phase: the last create has + // happened, so the listing is already authoritative, and every extra + // minute between reading the target's refs and acting on them is a minute + // in which another run can pick the marker up and start resuming from it. + resolvePendingMarkers(ctx, p, pending) + // Tail phase: tags and other-kind refs (issue #1) if len(tailPlans) > 0 { p.log("bootstrap batch pushing tail refs after branch batches", "tail_count", len(tailPlans)) if p.OnPhase != nil { p.OnPhase(tailPhaseLabel(tailPlans)) } - tailTargetRefs := planner.CopyRefHashMap(p.TargetRefs) - for _, batch := range batches { - tailTargetRefs[batch.Plan.TargetRef] = batch.Plan.SourceHash - } + // completedRefs, not a fresh pass over `batches`: it is the map this + // run maintains as what the target demonstrably holds — branch tips + // whose creates were confirmed, and the temp refs still anchoring the + // ones that were not. Re-deriving from the plans here would claim the + // creates the cutover deliberately withheld. + tailTargetRefs := planner.CopyRefHashMap(completedRefs) packReader, err := p.SourceService.FetchPack(ctx, p.SourceConn, tailDesired, tailTargetRefs) if err != nil { if errors.Is(err, git.NoErrAlreadyUpToDate) { @@ -713,6 +903,96 @@ func executeBatched( //nolint:maintidx // complex batch logic is inherently bran return result, nil } +// resolvePendingMarkers settles the markers whose branch create no push could +// confirm, by asking the target which branches it actually has. A marker whose +// branch is present at the hash this run pushed is stale scaffolding and is +// deleted; anything else is kept, with the reason stated. When the target +// cannot be asked, everything is kept: one run's worth of leftover scaffolding +// is recoverable, a discarded resume position is a full re-transfer. +// +// Deliberately not fatal. The branches that did land are on the target, the +// operator asked for those, and a listing failure at the very end of a +// multi-gigabyte import must not turn a delivered import into a failed run. +func resolvePendingMarkers(ctx context.Context, p Params, pending []pendingMarker) { + if len(pending) == 0 { + return + } + if p.TargetRefsNow == nil { + for _, marker := range pending { + keepMarker(p, marker, "no way to list the target's refs") + } + return + } + refsNow, err := p.TargetRefsNow(ctx) + if err != nil { + for _, marker := range pending { + keepMarker(p, marker, fmt.Sprintf("could not list the target's refs: %v", err)) + } + return + } + + cmds := make([]gitproto.PushCommand, 0, len(pending)) + deleted := make([]pendingMarker, 0, len(pending)) + for _, marker := range pending { + switch onTarget := refsNow[marker.branch]; { + case onTarget.IsZero(): + keepMarker(p, marker, fmt.Sprintf("%s is not on the target", marker.branch)) + case onTarget != marker.createHash: + // Someone else's branch, at their commit. Everything between it + // and this import's tip is reachable from the marker and nothing + // else, so the marker is still the only thing holding it. + keepMarker(p, marker, fmt.Sprintf("%s is on the target at %s, not the %s this run pushed", + marker.branch, planner.ShortHash(onTarget), planner.ShortHash(marker.createHash))) + default: + p.log("bootstrap deleting resume marker; branch present at pushed hash", + "branch", marker.branch.String(), "temp_ref", marker.tempRef.String(), + "target_hash", planner.ShortHash(onTarget), "doubt", marker.doubt) + cmds = append(cmds, gitproto.PushCommand{Name: marker.tempRef, Old: marker.resumeHash, Delete: true}) + deleted = append(deleted, marker) + } + } + if len(cmds) == 0 { + return + } + if err := p.TargetPusher.PushCommands(ctx, cmds); err != nil { + p.log("bootstrap resume marker cleanup failed; markers left on target", + "marker_count", len(cmds), "error", err.Error()) + p.notice(fmt.Sprintf("could not delete %d stale resume marker(s) — remove them by hand or let a --prune run reap them", len(cmds))) + return + } + // Per marker, because best-effort swallows a per-ref "ng" here exactly as + // it does at the cutover: a delete the target refused, or one whose Old no + // longer matches because a concurrent run moved the marker, returns nil. + for _, marker := range deleted { + if reason, refused := p.refRefused(marker.tempRef); refused { + p.log("bootstrap temp ref delete refused; marker left on target", + "branch", marker.branch.String(), "temp_ref", marker.tempRef.String(), + "reason", reason) + p.notice(fmt.Sprintf("target refused to delete %s (%s) — remove it by hand or let a --prune run reap it", + marker.tempRef, reason)) + } + } +} + +// keepMarker logs and surfaces a resume marker this run is leaving in place. +// +// On the replicate and sync routes the next --prune run reaps it once its +// branch lands, so a marker kept unnecessarily costs one run. On the bootstrap +// route it does not: Bootstrap() rejects --prune, so nothing here will ever +// remove a marker this function keeps, and against a target that neither +// reports per-ref status nor answers a ref listing it is permanent. Accepted +// deliberately, in that order of harm: a marker is inert scaffolding — the +// resume route requires the branch to be absent, so a stale one cannot +// misroute anything — while a marker deleted in error costs a full re-import. +func keepMarker(p Params, marker pendingMarker, why string) { + p.log("bootstrap keeping resume marker", + "branch", marker.branch.String(), "temp_ref", marker.tempRef.String(), + "resume_hash", planner.ShortHash(marker.resumeHash), + "doubt", marker.doubt, "reason", why) + p.notice(fmt.Sprintf("%s (%s) — keeping %s at %s so the next run resumes instead of re-importing", + why, marker.doubt, marker.tempRef, planner.ShortHash(marker.resumeHash))) +} + // tailPhaseLabel returns a phase label matching what's in plans. func tailPhaseLabel(plans []planner.BranchPlan) string { hasTag, hasOther := false, false diff --git a/internal/strategy/bootstrap/bootstrap_test.go b/internal/strategy/bootstrap/bootstrap_test.go index 6b83e8e6..8115835a 100644 --- a/internal/strategy/bootstrap/bootstrap_test.go +++ b/internal/strategy/bootstrap/bootstrap_test.go @@ -1755,3 +1755,392 @@ func writeLinearCommitChain(tb testing.TB, store storer.Storer, count int) []plu } return hashes } + +// forkedCommitGraph builds a commit graph with two divergent tips sharing a +// root: a linear trunk, plus one commit hanging off the trunk's second commit. +// A second branch whose tip is IN the trunk's ancestry is planned as subsumed +// and never fetches, so a test about later branches' fetch haves needs a fork. +func forkedCommitGraph(tb testing.TB, trunkLen int) (parents map[plumbing.Hash][]plumbing.Hash, trunkTip, forkTip plumbing.Hash) { + tb.Helper() + store := memory.NewStorage() + trunk := writeLinearCommitChain(tb, store, trunkLen) + obj := store.NewEncodedObject() + commit := &object.Commit{ + Author: object.Signature{Name: "test", Email: "test@example.com", When: time.Unix(9000, 0).UTC()}, + Committer: object.Signature{Name: "test", Email: "test@example.com", When: time.Unix(9000, 0).UTC()}, + Message: "fork", + TreeHash: plumbing.ZeroHash, + ParentHashes: []plumbing.Hash{trunk[1]}, + } + if err := commit.Encode(obj); err != nil { + tb.Fatalf("encode fork commit: %v", err) + } + forkTip, err := store.SetEncodedObject(obj) + if err != nil { + tb.Fatalf("store fork commit: %v", err) + } + return parentsFromCommitChainStore(tb, store), trunk[trunkLen-1], forkTip +} + +// reachableParents narrows a commit-parents map to the commits reachable from +// tip. +func reachableParents(parents map[plumbing.Hash][]plumbing.Hash, tip plumbing.Hash) map[plumbing.Hash][]plumbing.Hash { + out := map[plumbing.Hash][]plumbing.Hash{} + queue := []plumbing.Hash{tip} + for len(queue) > 0 { + hash := queue[0] + queue = queue[1:] + if _, seen := out[hash]; seen { + continue + } + out[hash] = parents[hash] + queue = append(queue, parents[hash]...) + } + return out +} + +// batchedRefusalHarness drives a two-branch batched bootstrap whose target +// answers with the given per-ref "ng" statuses, and records what the run +// fetched and pushed. +type batchedRefusalHarness struct { + params Params + trunkRef, forkRef plumbing.ReferenceName + trunkTempRef plumbing.ReferenceName + trunkTip plumbing.Hash + fetchHaves []map[plumbing.ReferenceName]plumbing.Hash + pushCommandsBatches [][]gitproto.PushCommand + // refsNow is what the target answers when the run asks which refs it + // actually has, and refsNowErr makes that question unanswerable. Together + // they stand in for the only authority on whether a branch landed. + refsNow map[plumbing.ReferenceName]plumbing.Hash + refsNowErr error + refsNowN int + // silent makes the target report nothing per ref, as one that never + // negotiated report-status does. + silent bool +} + +func newBatchedRefusalHarness(t *testing.T, refused map[plumbing.ReferenceName]string) *batchedRefusalHarness { + t.Helper() + return newBatchedHarness(t, refused, false) +} + +// newSubsumedRefusalHarness is the same two branches with the second one's tip +// inside the trunk's ancestry, so it is planned as subsumed: one ref create, no +// pack, no temp ref. +func newSubsumedRefusalHarness(t *testing.T, refused map[plumbing.ReferenceName]string) *batchedRefusalHarness { + t.Helper() + return newBatchedHarness(t, refused, true) +} + +func newBatchedHarness(t *testing.T, refused map[plumbing.ReferenceName]string, subsumeSecond bool) *batchedRefusalHarness { + t.Helper() + parents, trunkTip, forkTip := forkedCommitGraph(t, 4) + if subsumeSecond { + // A tip on the trunk's own chain: reachable from it, so trunk's + // batches deliver every object and the planner subsumes it. + for hash, ps := range parents { + if hash == trunkTip { + forkTip = ps[0] + } + } + } + h := &batchedRefusalHarness{ + trunkRef: plumbing.NewBranchReferenceName("main"), + forkRef: plumbing.NewBranchReferenceName("release"), + trunkTip: trunkTip, + refsNow: map[plumbing.ReferenceName]plumbing.Hash{}, + } + h.trunkTempRef = planner.BootstrapTempRef(h.trunkRef) + h.params = Params{ + SourceService: fakeBootstrapSource{ + fetchCommitParents: func(_ context.Context, _ gitproto.Conn, ref gitproto.DesiredRef, _ []plumbing.Hash) (map[plumbing.Hash][]plumbing.Hash, error) { + // Only the requested ref's ancestry, as a real commit-graph + // fetch answers: handing back the whole graph would put the + // fork tip in trunk's stop set and plan it as subsumed. + return reachableParents(parents, ref.SourceHash), nil + }, + fetchPack: func(_ context.Context, _ gitproto.Conn, _ map[plumbing.ReferenceName]gitproto.DesiredRef, haves map[plumbing.ReferenceName]plumbing.Hash) (io.ReadCloser, error) { + h.fetchHaves = append(h.fetchHaves, planner.CopyRefHashMap(haves)) + return io.NopCloser(bytes.NewReader([]byte("PACK"))), nil + }, + }, + TargetPusher: fakeBootstrapPusher{ + pushPack: func(_ context.Context, _ []gitproto.PushCommand, pack io.ReadCloser) error { + _ = pack.Close() + return nil + }, + pushCommands: func(_ context.Context, cmds []gitproto.PushCommand) error { + h.pushCommandsBatches = append(h.pushCommandsBatches, append([]gitproto.PushCommand(nil), cmds...)) + return nil + }, + }, + DesiredRefs: map[plumbing.ReferenceName]planner.DesiredRef{ + h.trunkRef: {SourceRef: h.trunkRef, TargetRef: h.trunkRef, SourceHash: trunkTip, Kind: planner.RefKindBranch, Label: "main"}, + h.forkRef: {SourceRef: h.forkRef, TargetRef: h.forkRef, SourceHash: forkTip, Kind: planner.RefKindBranch, Label: "release"}, + }, + TargetRefs: map[plumbing.ReferenceName]plumbing.Hash{}, + SourceHeadTarget: h.trunkRef, + TargetMaxPack: 1024 * 1024, + RefOutcome: func(name plumbing.ReferenceName) (gitproto.RefOutcome, string) { + if reason, ok := refused[name]; ok { + return gitproto.RefOutcomeRefused, reason + } + if h.silent { + return gitproto.RefOutcomeUnknown, "" + } + return gitproto.RefOutcomeApplied, "" + }, + TargetRefsNow: func(context.Context) (map[plumbing.ReferenceName]plumbing.Hash, error) { + h.refsNowN++ + if h.refsNowErr != nil { + return nil, h.refsNowErr + } + return h.refsNow, nil + }, + } + return h +} + +// deletedTrunkMarker reports whether the run pushed a delete for the trunk +// branch's resume marker. +func (h *batchedRefusalHarness) deletedTrunkMarker() bool { + for _, cmds := range h.pushCommandsBatches { + for _, cmd := range cmds { + if cmd.Name == h.trunkTempRef && cmd.Delete { + return true + } + } + } + return false +} + +// A refused branch create must leave the marker in place AND leave its hash +// usable as a fetch have for the branches planned after it — the reason the +// cutover records the temp ref instead of the branch it could not create. +func TestExecuteBatchedRefusedCreateKeepsMarkerAsHave(t *testing.T) { + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{ + plumbing.NewBranchReferenceName("main"): "deny creating a protected branch", + }) + + if _, err := Execute(context.Background(), h.params, "empty target"); err != nil { + t.Fatalf("Execute: %v", err) + } + + if h.deletedTrunkMarker() { + t.Errorf("deleted resume marker %s after a create the target refused: %v", + h.trunkTempRef, h.pushCommandsBatches) + } + // Kept because the target says the branch is absent, not because the ng + // text was read: the listing has to have been consulted. + if h.refsNowN == 0 { + t.Error("marker decided without asking the target which refs it has") + } + if len(h.fetchHaves) < 2 { + t.Fatalf("expected a fetch for the second branch, got %d fetch(es)", len(h.fetchHaves)) + } + // The second branch's fetch must offer the kept marker's hash, or the + // objects trunk already delivered are re-sent. + var offeredTrunkTip bool + for _, haves := range h.fetchHaves[1:] { + for _, hash := range haves { + if hash == h.trunkTip { + offeredTrunkTip = true + } + } + } + if !offeredTrunkTip { + t.Errorf("later fetch did not offer the kept marker at %s as a have: %v", + planner.ShortHash(h.trunkTip), h.fetchHaves) + } +} + +// The common path pays nothing: a confirmed create deletes its marker straight +// away, without asking the target for a ref listing. +func TestExecuteBatchedConfirmedCreateDeletesMarkerWithoutListing(t *testing.T) { + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{}) + + if _, err := Execute(context.Background(), h.params, "empty target"); err != nil { + t.Fatalf("Execute: %v", err) + } + if !h.deletedTrunkMarker() { + t.Errorf("did not delete resume marker %s after a confirmed create: %v", + h.trunkTempRef, h.pushCommandsBatches) + } + if h.refsNowN != 0 { + t.Errorf("asked the target for %d ref listing(s) on a run with nothing in doubt", h.refsNowN) + } +} + +// A refusal does not always mean the branch is absent — "already exists" says +// the opposite, and a pre-receive message can contain that phrase while the +// branch really is missing. So the decision is made on what the target has, +// not on what it wrote: branch present means the marker is stale scaffolding +// and must go, or it strands forever on the bootstrap route, which refuses +// --prune and so has no cleaner. +func TestExecuteBatchedRefusedButPresentBranchDeletesMarker(t *testing.T) { + trunkRef := plumbing.NewBranchReferenceName("main") + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{ + trunkRef: "already exists", + }) + // There, at the hash this run pushed — so the import did land and the + // scaffolding is genuinely spent. + h.refsNow[trunkRef] = h.trunkTip + + if _, err := Execute(context.Background(), h.params, "empty target"); err != nil { + t.Fatalf("Execute: %v", err) + } + if !h.deletedTrunkMarker() { + t.Errorf("kept resume marker %s for a branch the target has: %v", + h.trunkTempRef, h.pushCommandsBatches) + } +} + +// The mirror image, and the reason the text is not consulted: a refusal whose +// wording happens to contain "already exists" while the branch is genuinely +// absent must keep the marker. Classifying the prose would delete it and cost +// a full re-transfer. +func TestExecuteBatchedRefusalMentioningExistenceKeepsMarkerWhenAbsent(t *testing.T) { + trunkRef := plumbing.NewBranchReferenceName("main") + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{ + trunkRef: "refusing to create refs/heads/main: a tag with that name already exists", + }) + + if _, err := Execute(context.Background(), h.params, "empty target"); err != nil { + t.Fatalf("Execute: %v", err) + } + if h.deletedTrunkMarker() { + t.Errorf("deleted resume marker %s for a branch the target does not have: %v", + h.trunkTempRef, h.pushCommandsBatches) + } +} + +// Silence is not confirmation on the subsumed path either. Nothing is at stake +// but the count, so no listing is fetched for it — the create simply is not +// reported as delivered. Trunk's own checkpoint pack still counts, which is +// what separates the two numbers below. +func TestExecuteBatchedSubsumedUnconfirmedCreateNotCounted(t *testing.T) { + confirmed := newSubsumedRefusalHarness(t, map[plumbing.ReferenceName]string{}) + baseline, err := Execute(context.Background(), confirmed.params, "empty target") + if err != nil { + t.Fatalf("Execute (confirmed): %v", err) + } + + silent := newSubsumedRefusalHarness(t, map[plumbing.ReferenceName]string{}) + silent.silent = true + result, err := Execute(context.Background(), silent.params, "empty target") + if err != nil { + t.Fatalf("Execute (silent target): %v", err) + } + if result.BatchCount != baseline.BatchCount-1 { + t.Errorf("BatchCount=%d against a silent target, want %d — one less than the %d a confirmed run reports, "+ + "since the subsumed create is the only difference", + result.BatchCount, baseline.BatchCount-1, baseline.BatchCount) + } +} + +// A branch present at someone else's commit is not this import's branch. The +// span between their tip and ours is reachable from the marker and nothing +// else, so deleting it would strand exactly the objects this run delivered. +func TestExecuteBatchedBranchAtOtherHashKeepsMarker(t *testing.T) { + trunkRef := plumbing.NewBranchReferenceName("main") + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{ + trunkRef: "already exists", + }) + h.refsNow[trunkRef] = plumbing.NewHash("6dcf09a3e2a1b3d1d1c88f1ad5e63e3f3d1a2b3c") + + if _, err := Execute(context.Background(), h.params, "empty target"); err != nil { + t.Fatalf("Execute: %v", err) + } + if h.deletedTrunkMarker() { + t.Errorf("deleted resume marker %s though %s sits at another commit: %v", + h.trunkTempRef, trunkRef, h.pushCommandsBatches) + } +} + +// A target that refuses the temp ref itself has applied nothing: the run must +// stop rather than advance its checkpoint state against a hash the target +// never accepted. +func TestExecuteBatchedRefusedTempRefStopsTheRun(t *testing.T) { + trunkTempRef := planner.BootstrapTempRef(plumbing.NewBranchReferenceName("main")) + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{ + plumbing.NewBranchReferenceName("main"): "deny updating a hidden ref", + trunkTempRef: "deny updating a hidden ref", + }) + + _, err := Execute(context.Background(), h.params, "empty target") + if err == nil { + t.Fatal("Execute succeeded against a target that refused the temp ref") + } + if !strings.Contains(err.Error(), trunkTempRef.String()) { + t.Errorf("error does not name the refused temp ref: %v", err) + } + if h.deletedTrunkMarker() { + t.Errorf("deleted a marker the target never accepted: %v", h.pushCommandsBatches) + } +} + +// Silence from a target that never advertised report-status is not +// confirmation — but it is not a failure either. The listing settles it: the +// create landed, so the marker is stale and goes. Without this the marker +// would be permanent on the one route that forbids --prune. +func TestExecuteBatchedUnreportingTargetSettledByListing(t *testing.T) { + trunkRef := plumbing.NewBranchReferenceName("main") + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{}) + h.silent = true + h.refsNow[trunkRef] = h.trunkTip + + if _, err := Execute(context.Background(), h.params, "empty target"); err != nil { + t.Fatalf("Execute: %v", err) + } + if h.refsNowN == 0 { + t.Error("never asked the target which refs it has, so nothing was confirmed") + } + if !h.deletedTrunkMarker() { + t.Errorf("kept resume marker %s though the target has the branch: %v", + h.trunkTempRef, h.pushCommandsBatches) + } +} + +// When the target cannot be asked either, nothing is settled and everything is +// kept — one run's leftover scaffolding against a full re-transfer — and the +// run still succeeds, because the branches that landed are what the operator +// asked for. +func TestExecuteBatchedUnansweredListingKeepsMarker(t *testing.T) { + h := newBatchedRefusalHarness(t, map[plumbing.ReferenceName]string{}) + h.silent = true + h.refsNowErr = errors.New("target listing unavailable") + + if _, err := Execute(context.Background(), h.params, "empty target"); err != nil { + t.Fatalf("Execute must not fail because a listing failed: %v", err) + } + if h.deletedTrunkMarker() { + t.Errorf("deleted resume marker %s without confirming anything: %v", + h.trunkTempRef, h.pushCommandsBatches) + } +} + +// A subsumed branch has no temp ref to lose, but its create is a lone ref push +// with the same nil-error-is-not-proof problem: a refused create must not be +// counted as a delivered batch or offered as a have. +func TestExecuteBatchedSubsumedRefusedCreateNotCounted(t *testing.T) { + forkRef := plumbing.NewBranchReferenceName("release") + h := newSubsumedRefusalHarness(t, map[plumbing.ReferenceName]string{ + forkRef: "deny creating a protected branch", + }) + + result, err := Execute(context.Background(), h.params, "empty target") + if err != nil { + t.Fatalf("Execute: %v", err) + } + // Trunk's checkpoint only. Counting the refused subsumed create would + // report work the target never accepted. + if result.BatchCount != 1 { + t.Errorf("BatchCount=%d, want 1 (trunk only; the subsumed create was refused)", result.BatchCount) + } + for _, haves := range h.fetchHaves { + if _, ok := haves[forkRef]; ok { + t.Errorf("offered refused branch %s as a fetch have: %v", forkRef, haves) + } + } +} diff --git a/internal/syncer/integration_test.go b/internal/syncer/integration_test.go index d5159a72..bc209d83 100644 --- a/internal/syncer/integration_test.go +++ b/internal/syncer/integration_test.go @@ -10,6 +10,7 @@ import ( "net/http/httptest" "os" "sort" + "strconv" "strings" "sync" "testing" @@ -30,6 +31,7 @@ import ( "github.com/go-git/go-git/v6/plumbing/protocol/packp" "github.com/go-git/go-git/v6/plumbing/protocol/packp/sideband" "github.com/go-git/go-git/v6/plumbing/revlist" + "github.com/go-git/go-git/v6/plumbing/storer" "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-git/go-git/v6/storage/memory" ) @@ -1174,7 +1176,7 @@ func TestBootstrap_IntegrationBatchedDeleteFailureRecoversOnRetry(t *testing.T) report.UnpackStatus = "ok" report.CommandStatuses = append(report.CommandStatuses, &packp.CommandStatus{ ReferenceName: cmd.Name, - Status: "ng simulated temp-ref delete failure", + Status: "simulated temp-ref delete failure", }) return report } @@ -1255,7 +1257,7 @@ func TestBootstrap_IntegrationBatchedPackFailureResumesOnRetry(t *testing.T) { for _, cmd := range req.Commands { report.CommandStatuses = append(report.CommandStatuses, &packp.CommandStatus{ ReferenceName: cmd.Name, - Status: "ng simulated checkpoint pack failure", + Status: "simulated checkpoint pack failure", }) } return report @@ -1574,7 +1576,7 @@ func TestRun_IntegrationIncrementalPushFailureRecoversOnRetry(t *testing.T) { for _, cmd := range req.Commands { report.CommandStatuses = append(report.CommandStatuses, &packp.CommandStatus{ ReferenceName: cmd.Name, - Status: "ng simulated incremental push failure", + Status: "simulated incremental push failure", }) } return report @@ -2613,6 +2615,559 @@ func TestRun_IntegrationReplicateBootstrapBatchesWhenConfigured(t *testing.T) { assertHeadsMatch(t, sourceRepo, targetRepo, testBranch) } +// refCreateDenier is a receive-pack hook that refuses to create one ref while +// applying every other command in the same request for real. +// +// syncertest.DenyRefsReport cannot stand in: it builds statuses but applies +// nothing, and a non-nil report short-circuits the test server before it +// applies anything either. For a bootstrap cutover that would leave the temp +// ref at the previous checkpoint and make the delete that follows carry a +// stale Old — a shape a real receive-pack would reject, hiding the behaviour +// under test behind a second failure. Applying the rest keeps the sequence +// faithful: the marker genuinely reaches the final checkpoint, and is then +// genuinely destroyed or kept. +// +// Requires receivePackUnpackForHook on the server, since the objects behind +// the commands it applies arrive in the pack this report short-circuits. +type refCreateDenier struct { + repo *git.Repository + ref plumbing.ReferenceName + reason string + + // The httptest handler runs on its own goroutine while the test drives + // Run, so both fields below are shared state. + mu sync.Mutex + // off lets a later run through, so a test can assert what the retry does + // once the target stops refusing the create. + off bool + // denials counts requests answered with an ng for ref, so a test can + // confirm the create was actually attempted rather than never sent. + denials int +} + +// allow stops refusing the create. +func (d *refCreateDenier) allow() { + d.mu.Lock() + defer d.mu.Unlock() + d.off = true +} + +// denialCount returns how many requests have been answered with an ng. +func (d *refCreateDenier) denialCount() int { + d.mu.Lock() + defer d.mu.Unlock() + return d.denials +} + +func (d *refCreateDenier) hook(req *packp.UpdateRequests, _ bool) *packp.ReportStatus { + carriesRef := false + for _, cmd := range req.Commands { + if cmd.Name == d.ref { + carriesRef = true + } + } + d.mu.Lock() + skip := d.off || !carriesRef + if !skip { + d.denials++ + } + d.mu.Unlock() + if skip { + return nil // ordinary checkpoint and delete pushes apply normally + } + // Bare reasons: go-git encodes "ng ", so a status carrying + // its own "ng " prefix reaches the operator doubled. + return applyCommandsExcept(d.repo.Storer, req, d.ref, d.reason) +} + +// applyCommandsExcept applies every ref command in req to storer and reports +// the outcome per command, answering "ng " for except and enforcing +// each command's Old as a real receive-pack's compare-and-swap does — without +// which a fixture accepts the stale-Old updates it exists to catch. +func applyCommandsExcept( + storer storer.Storer, + req *packp.UpdateRequests, + except plumbing.ReferenceName, + reason string, +) *packp.ReportStatus { + report := &packp.ReportStatus{UnpackStatus: "ok"} + for _, cmd := range req.Commands { + status := "ok" + switch { + case cmd.Name == except: + status = reason + case cmd.Old != currentRefHash(storer, cmd.Name): + status = fmt.Sprintf("stale info, expected %s", cmd.Old) + case cmd.New.IsZero(): + if err := storer.RemoveReference(cmd.Name); err != nil { + status = err.Error() + } + default: + if err := storer.SetReference(plumbing.NewHashReference(cmd.Name, cmd.New)); err != nil { + status = err.Error() + } + } + report.CommandStatuses = append(report.CommandStatuses, &packp.CommandStatus{ + ReferenceName: cmd.Name, + Status: status, + }) + } + return report +} + +// currentRefHash returns a storer's value for name, zero when absent — the +// value a push command's Old must match. +func currentRefHash(s storer.Storer, name plumbing.ReferenceName) plumbing.Hash { + ref, err := s.Reference(name) + if err != nil { + return plumbing.ZeroHash + } + return ref.Hash() +} + +// unpackPushedObjects decodes the packfile in a receive-pack body, if any, into +// storer. +func unpackPushedObjects(s storer.Storer, body []byte) error { + offset := receivePackPackOffset(body) + if offset < 0 { + return nil + } + return packfile.UpdateObjectStorage(s, bytes.NewReader(body[offset:])) +} + +func TestReceivePackPackOffset(t *testing.T) { + pktLine := func(payload string) string { + return fmt.Sprintf("%04x%s", len(payload)+4, payload) + } + // A ref whose name contains the literal the old scan looked for. + cmd := pktLine("0000000000000000000000000000000000000000 1111111111111111111111111111111111111111 refs/heads/PACKAGING\x00report-status") + refsOnly := cmd + "0000" + withPack := refsOnly + "PACK\x00\x00\x00\x02" + + if got := receivePackPackOffset([]byte(refsOnly)); got != -1 { + t.Errorf("offset in a ref-only body = %d, want -1 (the ref name is not a packfile)", got) + } + if got, want := receivePackPackOffset([]byte(withPack)), len(refsOnly); got != want { + t.Errorf("offset = %d, want %d (just past the flush)", got, want) + } + if got := receivePackPackOffset([]byte("garbage")); got != -1 { + t.Errorf("offset in an undecodable body = %d, want -1", got) + } +} + +// receivePackPackOffset returns the index in a receive-pack request body where +// the packfile begins, or -1 when the request carries none. It walks the +// pkt-line framing to the flush that ends the command list, rather than +// searching the body for "PACK": a ref named refs/heads/PACKAGING would put +// that literal in the command list and yield an offset inside it. +func receivePackPackOffset(body []byte) int { + for i := 0; i+4 <= len(body); { + length, err := strconv.ParseUint(string(body[i:i+4]), 16, 32) + if err != nil { + return -1 + } + if length == 0 { // flush-pkt: the command list ends here + if i+4 >= len(body) { + return -1 + } + return i + 4 + } + if length < 4 || i+int(length) > len(body) { + return -1 + } + i += int(length) + } + return -1 +} + +func TestRun_IntegrationBatchedCutoverKeepsResumeMarkerOnRejectedCreate(t *testing.T) { + // A batched bootstrap's temp ref (refs/gitsync/bootstrap/heads/) + // is the only record of how far the import got. The cutover push carries + // two commands in one request — advance the temp ref to the final + // checkpoint, and create the real branch there — and the temp ref is + // deleted immediately afterwards on the strength of that push returning + // no error. + // + // Under BestEffort a nil push error does not mean the create landed: + // gitproto hands a per-ref "ng" to OnRejection and returns nil. So a + // target that refuses the branch create (protected branch, pre-receive + // policy) leaves the branch absent AND has its resume marker deleted, and + // the run reports success. Every object pushed so far is then unreferenced + // on the target: nothing points at it, so the next run has no fetch have + // to negotiate against and re-transfers the whole history — on precisely + // the large repositories batching exists for. + // + // The marker must survive a create this run cannot confirm. + sourceRepo, sourceFS := newSourceRepo(t) + makeLargeCommits(t, sourceRepo, sourceFS, 80, 5_000) + branchRef := plumbing.NewBranchReferenceName(testBranch) + tempRef := planner.BootstrapTempRef(branchRef) + + targetRepo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init target repo: %v", err) + } + + sourceServer := newSmartHTTPRepoServerV2(t, sourceRepo) + targetServer := newSmartHTTPRepoServer(t, targetRepo) + defer sourceServer.Close() + defer targetServer.Close() + + // Refuse only the branch create; every other command in that request is + // applied for real, objects included (see refCreateDenier). + denier := &refCreateDenier{repo: targetRepo, ref: branchRef, reason: "deny creating a protected branch"} + targetServer.receivePackHook = denier.hook + targetServer.receivePackUnpackForHook = true + + result, err := Run(context.Background(), Config{ + Source: Endpoint{URL: sourceServer.RepoURL()}, + Target: Endpoint{URL: targetServer.RepoURL()}, + Mode: modeReplicate, + AllRefs: true, + IncludeTags: true, + Prune: true, + BestEffort: true, + ProtocolMode: protocolModeAuto, + TargetMaxPackBytes: 350_000, // force > 1 batch, so a checkpoint exists to lose + }) + if err != nil { + t.Fatalf("expected the best-effort run to succeed despite the rejected create: %v", err) + } + + // Test setup, not the property under test: without these the assertion + // below would pass for the wrong reasons. + if denier.denialCount() == 0 { + t.Fatal("test setup: the cutover push never carried the branch create") + } + if result.BatchCount < 2 { + t.Fatalf("test setup: expected a batched bootstrap with checkpoints to lose, got batch_count=%d", result.BatchCount) + } + if _, err := targetRepo.Reference(branchRef, true); err == nil { + t.Fatalf("test setup: expected %s to stay absent after the rejected create", branchRef) + } + + // The rejection itself has to reach the operator; it always did, and a + // marker kept silently would be its own bug. + if result.Warned != 1 || result.Pushed != 0 { + t.Errorf("rejected create reported as pushed=%d warned=%d, want 0/1", result.Pushed, result.Warned) + } + var warnedPlan bool + for _, plan := range result.Plans { + if plan.TargetRef != branchRef { + continue + } + warnedPlan = true + if plan.Action != ActionWarn { + t.Errorf("branch plan Action=%s, want warn", plan.Action) + } + // go-git encodes "ng ", so a status that carries its own + // "ng " reaches the operator doubled — the fixture must not. + if !strings.Contains(plan.Reason, "deny creating a protected branch") || strings.Contains(plan.Reason, "ng deny") { + t.Errorf("branch plan Reason=%q, want the target's bare ng reason", plan.Reason) + } + } + if !warnedPlan { + t.Errorf("no plan for %s in the result", branchRef) + } + + marker, err := targetRepo.Reference(tempRef, true) + if err != nil { + t.Fatalf("resume marker %s was deleted after a branch create the run could not confirm: %v\n"+ + "the run reported success (pushed=%d warned=%d, %d checkpoint packs) while leaving the target "+ + "with neither the branch nor the marker, so the next run re-transfers the whole history", + tempRef, err, result.Pushed, result.Warned, result.BatchCount) + } + // A marker is only a resume position if the objects under it are on the + // target; a ref at a hash the target cannot resolve resumes nothing. + if _, err := targetRepo.CommitObject(marker.Hash()); err != nil { + t.Fatalf("resume marker %s points at %s, which the target cannot resolve: %v", + tempRef, planner.ShortHash(marker.Hash()), err) + } +} + +func TestRun_IntegrationBatchedCutoverResumesAfterRejectedCreate(t *testing.T) { + // The other half of the property: a marker kept through a rejected create + // has to be worth keeping. Once the target accepts the branch, the retry + // must finish the import from the marker — no checkpoint re-pushed, no + // history re-fetched — and only then delete it. + sourceRepo, sourceFS := newSourceRepo(t) + makeLargeCommits(t, sourceRepo, sourceFS, 80, 5_000) + branchRef := plumbing.NewBranchReferenceName(testBranch) + tempRef := planner.BootstrapTempRef(branchRef) + sourceHead, err := sourceRepo.Reference(branchRef, true) + if err != nil { + t.Fatalf("resolve source head: %v", err) + } + + targetRepo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init target repo: %v", err) + } + + sourceServer := newSmartHTTPRepoServerV2(t, sourceRepo) + targetServer := newSmartHTTPRepoServer(t, targetRepo) + defer sourceServer.Close() + defer targetServer.Close() + + // Refuse the create on the first run, then let it through on the second. + denier := &refCreateDenier{repo: targetRepo, ref: branchRef, reason: "deny creating a protected branch"} + targetServer.receivePackHook = denier.hook + targetServer.receivePackUnpackForHook = true + + cfg := Config{ + Source: Endpoint{URL: sourceServer.RepoURL()}, + Target: Endpoint{URL: targetServer.RepoURL()}, + Mode: modeReplicate, + AllRefs: true, + IncludeTags: true, + Prune: true, + BestEffort: true, + ProtocolMode: protocolModeAuto, + TargetMaxPackBytes: 350_000, + } + + first, err := Run(context.Background(), cfg) + if err != nil { + t.Fatalf("first run: %v", err) + } + if first.BatchCount < 2 || denier.denialCount() == 0 { + t.Fatalf("test setup: expected a batched bootstrap with a refused create, got batch_count=%d denials=%d", + first.BatchCount, denier.denialCount()) + } + marker, err := targetRepo.Reference(tempRef, true) + if err != nil { + t.Fatalf("resume marker %s missing after the rejected create: %v", tempRef, err) + } + // At the final checkpoint, not the penultimate one: the whole import is + // what the retry must be able to skip, including the pack the create rode. + if marker.Hash() != sourceHead.Hash() { + t.Fatalf("resume marker at %s, want the final checkpoint %s — the retry would re-push the last batch", + planner.ShortHash(marker.Hash()), planner.ShortHash(sourceHead.Hash())) + } + if got, want := assertHistoryCompleteFrom(t, targetRepo, marker.Hash()), commitCount(t, sourceRepo, branchRef); got != want { + t.Errorf("marker reaches %d commits on the target, source has %d — the import is short a batch", got, want) + } + // Measured on the target, not the source: what a lost marker costs is the + // re-push of everything already delivered. (The source side is not + // measurable here — this test server advertises the fetch filter but + // ignores it, so even a commit-graph fetch comes back whole.) + firstBytesIn := targetServer.BytesIn(serviceReceivePack, metricPack) + targetServer.ResetMetrics() + + denier.allow() + + second, err := Run(context.Background(), cfg) + if err != nil { + t.Fatalf("resuming run: %v", err) + } + if second.RelayReason != planner.ReasonBootstrapResumeMarker { + t.Errorf("resuming run routed with reason %q, want %q", second.RelayReason, planner.ReasonBootstrapResumeMarker) + } + if second.BatchCount != 0 { + t.Errorf("resuming run pushed %d checkpoint pack(s), want 0: the marker already held the tip, "+ + "so only the branch create was outstanding", second.BatchCount) + } + if secondBytesIn := targetServer.BytesIn(serviceReceivePack, metricPack); secondBytesIn*20 >= firstBytesIn { + t.Errorf("resuming run sent the target %d bytes against the first run's %d — the import was re-done, not resumed", + secondBytesIn, firstBytesIn) + } + assertHeadsMatch(t, sourceRepo, targetRepo, testBranch) + // assertHeadsMatch compares hashes only, so the objects behind the branch + // are checked separately: a target short one batch would still match. + assertCommitHistoryPresent(t, targetRepo, sourceRepo, branchRef) + if _, err := targetRepo.Reference(tempRef, true); err == nil { + t.Errorf("resume marker %s outlived the branch it was scaffolding for", tempRef) + } +} + +func TestRun_IntegrationBatchedCutoverSettlesMarkerAgainstUnreportingTarget(t *testing.T) { + // A target that does not advertise report-status tells the client nothing + // about any command it sent: the pusher decodes no report, so every ref + // looks unrejected whether it landed or not. The cutover must not read + // that silence as confirmation — but it must not strand the marker either, + // because the bootstrap route forbids --prune and nothing else would ever + // reap it. It asks the target which refs it has instead. + sourceRepo, sourceFS := newSourceRepo(t) + makeLargeCommits(t, sourceRepo, sourceFS, 80, 5_000) + branchRef := plumbing.NewBranchReferenceName(testBranch) + tempRef := planner.BootstrapTempRef(branchRef) + + targetRepo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init target repo: %v", err) + } + + sourceServer := newSmartHTTPRepoServerV2(t, sourceRepo) + targetServer := newSmartHTTPRepoServer(t, targetRepo) + targetServer.receivePackNoReportStatus = true + defer sourceServer.Close() + defer targetServer.Close() + + result, err := Run(context.Background(), Config{ + Source: Endpoint{URL: sourceServer.RepoURL()}, + Target: Endpoint{URL: targetServer.RepoURL()}, + Mode: modeReplicate, + AllRefs: true, + IncludeTags: true, + Prune: true, + BestEffort: true, + ProtocolMode: protocolModeAuto, + TargetMaxPackBytes: 350_000, + }) + if err != nil { + t.Fatalf("batched bootstrap against a target without report-status: %v", err) + } + if result.BatchCount < 2 { + t.Fatalf("test setup: expected a batched bootstrap, got batch_count=%d", result.BatchCount) + } + if _, err := targetRepo.Reference(branchRef, true); err != nil { + t.Fatalf("branch %s missing after a run the target accepted: %v", branchRef, err) + } + assertCommitHistoryPresent(t, targetRepo, sourceRepo, branchRef) + // The create did land; the target simply could not say so. Having asked, + // the run knows the scaffolding is stale. + if _, err := targetRepo.Reference(tempRef, true); err == nil { + t.Errorf("resume marker %s left behind on a route that cannot prune it", tempRef) + } +} + +func TestRun_IntegrationBatchedCutoverSanitizesRejectionInNotice(t *testing.T) { + // The ng reason is free-form text the target wrote, and the marker-kept + // notice prints it. An escape sequence in it could clear the line the + // warning was drawn on and redraw it as a success — the attack + // internal/sanitize exists for — so the reason must be filtered on the way + // to the terminal, not just on the way into a plan's Reason. + const hostile = "denied\x1b[1A\x1b[2Kbranch created ok" + sourceRepo, sourceFS := newSourceRepo(t) + makeLargeCommits(t, sourceRepo, sourceFS, 80, 5_000) + branchRef := plumbing.NewBranchReferenceName(testBranch) + + targetRepo, err := git.Init(memory.NewStorage()) + if err != nil { + t.Fatalf("init target repo: %v", err) + } + + sourceServer := newSmartHTTPRepoServerV2(t, sourceRepo) + targetServer := newSmartHTTPRepoServer(t, targetRepo) + denier := &refCreateDenier{repo: targetRepo, ref: branchRef, reason: hostile} + targetServer.receivePackHook = denier.hook + targetServer.receivePackUnpackForHook = true + defer sourceServer.Close() + defer targetServer.Close() + + var progressBuf bytes.Buffer + if _, err := Run(context.Background(), Config{ + Source: Endpoint{URL: sourceServer.RepoURL()}, + Target: Endpoint{URL: targetServer.RepoURL()}, + Mode: modeReplicate, + AllRefs: true, + IncludeTags: true, + Prune: true, + BestEffort: true, + ProtocolMode: protocolModeAuto, + TargetMaxPackBytes: 350_000, + Progress: true, + progressOut: &progressBuf, + }); err != nil { + t.Fatalf("best-effort run: %v", err) + } + + printed := progressBuf.String() + if !strings.Contains(printed, "keeping") { + t.Fatalf("expected the marker-kept notice in the progress output, got %q", printed) + } + // The words survive; the cursor movement does not. + if !strings.Contains(printed, "branch created ok") { + t.Errorf("sanitizing must keep the text, only drop control characters: %q", printed) + } + if strings.Contains(printed, "\x1b[1A") || strings.Contains(printed, "\x1b[2K") { + t.Errorf("server-authored escape sequence reached the terminal: %q", printed) + } +} + +// assertHistoryCompleteFrom walks every commit reachable from tip in repo, +// failing if any commit, tree, or blob is missing, and returns the commit +// count. A ref can point at a hash the target never received, and a pack can +// deliver commits and trees whose blobs never resolved, so hash equality on +// the tip establishes neither. +func assertHistoryCompleteFrom(t *testing.T, repo *git.Repository, tip plumbing.Hash) int { + t.Helper() + seen := make(map[plumbing.Hash]bool) + queue := []plumbing.Hash{tip} + commits := 0 + for len(queue) > 0 { + hash := queue[0] + queue = queue[1:] + if seen[hash] { + continue + } + seen[hash] = true + commit, err := repo.CommitObject(hash) + if err != nil { + t.Fatalf("commit %s missing (%d reachable commits checked): %v", + planner.ShortHash(hash), commits, err) + } + commits++ + assertTreeComplete(t, repo, commit.TreeHash, hash) + queue = append(queue, commit.ParentHashes...) + } + if commits == 0 { + t.Fatalf("no commits reachable from %s", planner.ShortHash(tip)) + } + return commits +} + +// commitCount counts the commits reachable from ref in repo. +func commitCount(t *testing.T, repo *git.Repository, ref plumbing.ReferenceName) int { + t.Helper() + tip, err := repo.Reference(ref, true) + if err != nil { + t.Fatalf("resolve %s: %v", ref, err) + } + iter, err := repo.Log(&git.LogOptions{From: tip.Hash()}) + if err != nil { + t.Fatalf("walk history from %s: %v", ref, err) + } + count := 0 + if err := iter.ForEach(func(*object.Commit) error { count++; return nil }); err != nil { + t.Fatalf("count commits from %s: %v", ref, err) + } + return count +} + +// assertCommitHistoryPresent asserts the target holds ref, and every commit, +// tree and blob reachable from it, in the same quantity the source has. +func assertCommitHistoryPresent(t *testing.T, target, source *git.Repository, ref plumbing.ReferenceName) { + t.Helper() + tipRef, err := target.Reference(ref, true) + if err != nil { + t.Fatalf("target is missing %s: %v", ref, err) + } + if got, want := assertHistoryCompleteFrom(t, target, tipRef.Hash()), commitCount(t, source, ref); got != want { + t.Errorf("target holds %d commits reachable from %s, source has %d", got, ref, want) + } +} + +// assertTreeComplete resolves a tree and everything it names, recursively. +func assertTreeComplete(t *testing.T, repo *git.Repository, treeHash, commitHash plumbing.Hash) { + t.Helper() + tree, err := repo.TreeObject(treeHash) + if err != nil { + t.Fatalf("tree %s of commit %s missing from target: %v", + planner.ShortHash(treeHash), planner.ShortHash(commitHash), err) + } + for _, entry := range tree.Entries { + if entry.Mode.IsFile() { + if _, err := repo.BlobObject(entry.Hash); err != nil { + t.Fatalf("blob %s (%s) of commit %s missing from target: %v", + planner.ShortHash(entry.Hash), entry.Name, planner.ShortHash(commitHash), err) + } + continue + } + assertTreeComplete(t, repo, entry.Hash, commitHash) + } +} + func TestRun_IntegrationReplicateBootstrapsEmptyTarget(t *testing.T) { sourceRepo, sourceFS := newSourceRepo(t) makeCommits(t, sourceRepo, sourceFS, 2) @@ -2707,7 +3262,7 @@ func TestRun_IntegrationReplicateResumesInterruptedBatchedBootstrap(t *testing.T for _, cmd := range req.Commands { report.CommandStatuses = append(report.CommandStatuses, &packp.CommandStatus{ ReferenceName: cmd.Name, - Status: "ng simulated checkpoint pack failure", + Status: "simulated checkpoint pack failure", }) } return report @@ -4157,9 +4712,22 @@ type smartHTTPRepoServer struct { receivePackThinCap bool commandHook func(*packp.UpdateRequests) *packp.ReportStatus receivePackHook func(*packp.UpdateRequests, bool) *packp.ReportStatus - uploadPackRaw func(http.ResponseWriter, *http.Request, []byte) bool - uploadPackV2FetchRaw func(http.ResponseWriter, v2TestCommandRequest, []byte) bool - receivePackRaw func(http.ResponseWriter, *http.Request) bool + // receivePackUnpackForHook decodes a pushed packfile into the target + // storer before receivePackHook answers with its own report. A hook that + // returns a report short-circuits transport.ReceivePack, which is what + // would otherwise have unpacked the objects — so a hook that applies some + // of the request's ref commands itself needs this, or it leaves those refs + // pointing at objects the target does not have and every assertion about + // them passes on hashes alone. Objects for the refused command land too, + // which is what a real receive-pack does whenever any command in the + // request succeeds. + receivePackUnpackForHook bool + // receivePackNoReportStatus drops report-status from the receive-pack + // advertisement, so the client cannot learn any per-ref outcome. + receivePackNoReportStatus bool + uploadPackRaw func(http.ResponseWriter, *http.Request, []byte) bool + uploadPackV2FetchRaw func(http.ResponseWriter, v2TestCommandRequest, []byte) bool + receivePackRaw func(http.ResponseWriter, *http.Request) bool mu sync.Mutex metrics []exchangeMetric @@ -4320,7 +4888,7 @@ func (s *smartHTTPRepoServer) handleInfoRefs(w http.ResponseWriter, r *http.Requ return } - if service == serviceReceivePack && (s.receivePackNoThin || s.receivePackThinCap) { + if service == serviceReceivePack && (s.receivePackNoThin || s.receivePackThinCap || s.receivePackNoReportStatus) { rewritten, err := rewriteReceivePackAdvertisement(buf.Bytes(), func(caps *capability.List) { if s.receivePackThinCap { caps.Delete(capability.Capability("no-thin")) @@ -4328,6 +4896,9 @@ func (s *smartHTTPRepoServer) handleInfoRefs(w http.ResponseWriter, r *http.Requ if s.receivePackNoThin { caps.Set(capability.Capability("no-thin")) } + if s.receivePackNoReportStatus { + caps.Delete(capability.ReportStatus) + } }) if err != nil { s.tb.Fatalf("rewrite receive-pack advertisement: %v", err) @@ -4605,7 +5176,7 @@ func (s *smartHTTPRepoServer) handleReceivePack(w http.ResponseWriter, r *http.R return } - hasPack := bytes.Contains(body, []byte("PACK")) + hasPack := receivePackPackOffset(body) >= 0 // For no-PACK requests, handle manually since transport.ReceivePack // expects a packfile when there are create/update commands. @@ -4628,6 +5199,14 @@ func (s *smartHTTPRepoServer) handleReceivePack(w http.ResponseWriter, r *http.R return } } + if s.receivePackNoReportStatus { + // A target that advertised no report-status must not answer with + // one here either, or half its requests would quietly be + // reporting — delete-only pushes carry no pack and land in this + // branch. + s.applyReceivePackBody(w, body) + return + } report := &packp.ReportStatus{} report.UnpackStatus = "ok" @@ -4658,12 +5237,29 @@ func (s *smartHTTPRepoServer) handleReceivePack(w http.ResponseWriter, r *http.R http.Error(w, err.Error(), http.StatusBadRequest) return } + if s.receivePackUnpackForHook { + if err := unpackPushedObjects(s.repo.Storer, body); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + } if report := s.receivePackHook(req, true); report != nil { s.writeReceivePackReport(w, report, &req.Capabilities, len(body)) return } } + if s.receivePackNoReportStatus { + // go-git's server-side ReceivePack returns as soon as it has unpacked + // the objects when the client does not support report-status — it has + // nowhere to report the outcome, so it never applies the ref commands. + // A real receive-pack applies them regardless of what the client can + // be told, which is the whole point of a target that cannot report, + // so supply that here. + s.applyReceivePackBody(w, body) + return + } + var buf bytes.Buffer reader := io.NopCloser(bytes.NewReader(body)) writer := nopWriteCloser{&buf} @@ -4682,6 +5278,32 @@ func (s *smartHTTPRepoServer) handleReceivePack(w http.ResponseWriter, r *http.R s.recordMetric(serviceReceivePack, metricPack, int64(len(body)), int64(buf.Len()), 0, 0) } +// applyReceivePackBody unpacks a receive-pack request's objects and applies +// its ref commands, answering with an empty body — what a client that did not +// negotiate report-status expects. Used only where go-git's server-side +// ReceivePack declines to apply the commands itself. +func (s *smartHTTPRepoServer) applyReceivePackBody(w http.ResponseWriter, body []byte) { + req := &packp.UpdateRequests{} + if err := req.Decode(bytes.NewReader(body)); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + if err := unpackPushedObjects(s.repo.Storer, body); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + // Same compare-and-swap fidelity as the hook path; "" denies nothing. + report := applyCommandsExcept(s.repo.Storer, req, "", "") + for _, cs := range report.CommandStatuses { + if cs.Status != "ok" { + http.Error(w, fmt.Sprintf("%s: %s", cs.ReferenceName, cs.Status), http.StatusInternalServerError) + return + } + } + w.Header().Set("Content-Type", fmt.Sprintf("application/x-%s-result", serviceReceivePack)) + s.recordMetric(serviceReceivePack, metricPack, int64(len(body)), 0, 0, 0) +} + // writeReceivePackReport encodes a report-status, optionally wrapped in // sideband (matching what transport.ReceivePack would produce), and writes it. func (s *smartHTTPRepoServer) writeReceivePackReport(w http.ResponseWriter, report *packp.ReportStatus, caps *capability.List, bodyLen int) { diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 95dc50a3..f26f063e 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -597,6 +597,52 @@ func (s *syncSession) leaseFailureError() error { return fmt.Errorf("lease failure on %d ref(s) (%s) — target moved during sync; rerun, or use --force-blind to overwrite: %w", len(refs), strings.Join(refs, ", "), gitproto.ErrTargetRefMoved) } +// refOutcome reports what the target did with name in the push that just +// returned: applied it, refused it, or said nothing about it. Answers +// bootstrap.Params.RefOutcome, whose doc has the why; read after a push +// returns, from the same goroutine the pusher's callback ran on. +// +// Scoped to that one push rather than to s.rejections, which accumulates over +// the session: a strategy asking "did the ref I just pushed land" must not be +// answered with a rejection from an earlier request for the same name. +// +// The reason is sanitized here, as the OnRejection callback does for +// s.rejections: it is free-form text the target wrote, and from here it reaches +// the terminal in a notice and an error message, where an escape sequence could +// redraw the line a warning was printed on. +func (s *syncSession) refOutcome(name plumbing.ReferenceName) (gitproto.RefOutcome, string) { + outcome, reason := s.target.pusher.LastOutcome(name) + return outcome, sanitize.Text(reason) +} + +// targetRefsNow re-reads the target's refs mid-run. Answers +// bootstrap.Params.TargetRefsNow, which asks it only to settle whether a branch +// whose create no push could confirm is actually there — one advertisement +// round trip against a transfer measured in gigabytes. +// +// It is the receive-pack advertisement, so receive.hideRefs can omit a ref that +// is really there (the same caveat Config.TargetAssertedEmpty documents). That +// direction is safe for the one question asked here: a hidden branch reads as +// absent, which keeps a resume marker that was not needed rather than dropping +// one that was. +func (s *syncSession) targetRefsNow(ctx context.Context) (map[plumbing.ReferenceName]plumbing.Hash, error) { + if s.target == nil || s.target.conn == nil { + return nil, errors.New("no target connection") + } + adv, err := gitproto.AdvertisedRefsV1(ctx, s.target.conn, transport.ReceivePackService) + if err != nil { + return nil, fmt.Errorf("list target refs: %w", err) + } + // Skipped names are dropped without a second warning: newSession already + // reported them for this target, and a name git would reject cannot be one + // of the branches this run created. + refs, _, err := gitproto.AdvRefsToSlice(adv) + if err != nil { + return nil, fmt.Errorf("decode target refs: %w", err) + } + return gitproto.RefHashMap(refs), nil +} + // applyRejections downgrades plans whose ref was rejected by the target to // ActionWarn and returns the count. func (s *syncSession) applyRejections(plans []BranchPlan) int { @@ -1396,9 +1442,14 @@ func bootstrapWithInputs( SourceHeadTarget: s.sourceService.HeadTarget, MaxPackBytes: s.cfg.MaxPackBytes, TargetMaxPack: s.cfg.TargetMaxPackBytes, Verbose: s.cfg.Verbose, Logger: s.logger, - Strategy: s.cfg.BootstrapStrategy, - OnPhase: s.stats.setPhase, - OnNotice: s.notice, + Strategy: s.cfg.BootstrapStrategy, + OnPhase: s.stats.setPhase, + OnNotice: s.notice, + RefOutcome: s.refOutcome, + // Where a push leaves a create in doubt — refused, or a target that + // reports nothing — the strategy asks the target which refs it has + // rather than guessing from the wording of a rejection. + TargetRefsNow: s.targetRefsNow, }, relayReason) if err != nil { return Result{}, fmt.Errorf("bootstrap execute: %w", err)