Drain go-git upload-pack double-NAK before sideband demux · Entire

Drain go-git upload-pack double-NAK before sideband demux

b5fdb49→main·

Soph·3mo ago·3 files·+88 added/-5 removed

Root cause: go-git's v6 upload-pack server (plumbing/transport/upload_pack.go) emits two NAK pktlines when the client sends haves but none are reachable from the wants. The first NAK is written via ServerResponse.Encode for the "if len(haves) > 0" branch (empty ACKs => NAK), the second via the "no common ack found" branch. The response looks like:

0008NAK\n
0008NAK\n
0009\x01PACK... (sideband channel 1)

go-git's ServerResponse.Decode returns after consuming the first NAK, so the second NAK sits in front of the sideband-wrapped pack. The sideband demuxer then reads "NAK\n" as a frame and fails with "unknown channel NAK" (channel byte 'N' = 0x4e) because valid channels are 0x01/0x02/0x03.

This reliably hits any fetch where the target's advertised ref is not an ancestor of the source's want - exactly the rewind/overwrite scenario replicate mode is designed for. The same bug affected sync --force via the materialized strategy, but was masked in existing tests because multi-ref scenarios happened to include at least one have whose hash was reachable from a want, producing an ACK and skipping the two-NAK branch.

Fix: wrap the post-Decode reader in bufio.Reader and drain any extra "0008NAK\n" pktlines before handing off to the sideband demuxer. Applied to both fetchToStoreV1 (materialized path) and fetchPackV1 (relay path). A short stream that cannot satisfy the 8-byte peek is treated as "no more NAKs" so the downstream reader surfaces the real read error.

Also reverts the V1->V2 workaround in TestRun_IntegrationReplicateOverwrites-DivergentBranch now that V1 handles this scenario correctly, and adds TestFetchPackV1DrainsSecondNAK asserting the drainer's behavior directly.

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

Sessions

a8de9a2a91c3View transcript

Changes

3

1
2
3
4
5
6
7
333 unmodified lines

341
342
343
344
345
344
346
347
348
347
349
350
351
352
353
354
355
13 unmodified lines

369
370
371
372
373
368
374
375
376
377
378
379
380
381
382
373
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413

package gitproto

import (
    "bufio"
    "bytes"
    "context"
    "errors"
333 unmodified lines

defer ioutil.CheckClose(reader, &err)

// Decode server response (ACK/NAK) then read pack with sideband demux.
    buffered := bufio.NewReader(reader)
    var srvResp packp.ServerResponse
    if decErr := srvResp.Decode(reader); decErr != nil {
    if decErr := srvResp.Decode(buffered); decErr != nil {
        return fmt.Errorf("decode server response: %w", decErr)
    }
    sbReader := buildSidebandReader(caps, reader, nil)
    if drainErr := drainTrailingNAKs(buffered); drainErr != nil {
        return fmt.Errorf("drain server response: %w", drainErr)
    }
    sbReader := buildSidebandReader(caps, buffered, nil)
    return packfile.UpdateObjectStorage(store, sbReader)
}

13 unmodified lines

return nil, fmt.Errorf("source upload-pack: %w", err)
}

buffered := bufio.NewReader(reader)
    var srvResp packp.ServerResponse
    if decErr := srvResp.Decode(reader); decErr != nil {
    if decErr := srvResp.Decode(buffered); decErr != nil {
        _ = reader.Close()
        return nil, fmt.Errorf("decode server response: %w", decErr)
    }
    if drainErr := drainTrailingNAKs(buffered); drainErr != nil {
        _ = reader.Close()
        return nil, fmt.Errorf("drain server response: %w", drainErr)
    }
    return &wrappedRC{
        Reader: buildSidebandReader(caps, reader, nil),
        Reader: buildSidebandReader(caps, buffered, nil),
        Closer: reader,
    }, nil
}

// drainTrailingNAKs consumes any extra "NAK\n" pktlines left in the stream
// after ServerResponse.Decode. go-git's upload-pack server emits a second NAK
// when haves were sent but none were reachable from the wants (see
// plumbing/transport/upload_pack.go), while go-git's ServerResponse.Decode
// stops after the first NAK. The remainder would otherwise be misread by the
// sideband demuxer as a frame with channel byte 'N' ("unknown channel NAK").
//
// A stream that runs out before we can peek 8 bytes carries no trailing NAK
// to drain, so we silently stop. The downstream consumer observes the same
// underlying read error on its first read.
func drainTrailingNAKs(r *bufio.Reader) error {
    for {
        header, err := r.Peek(8)
        if len(header) < 8 || !bytes.Equal(header, []byte("0008NAK\n")) {
            _ = err
            return nil
        }
        if _, err := r.Discard(8); err != nil {
            return err
        }
    }
}

// buildSidebandReader wraps a reader with sideband demuxing if the negotiated
// capabilities include sideband support. Delegates to PreferredSideband (issue #4).
func buildSidebandReader(caps *capability.List, reader io.Reader, progress sideband.Progress) io.Reader {

Minternal/gitproto/fetch.go+37/-4

669 unmodified lines

670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725

669 unmodified lines

}
}

func TestFetchPackV1DrainsSecondNAK(t *testing.T) {
    // go-git's upload-pack server emits two NAKs when haves were sent but none
    // were reachable from the wants. ServerResponse.Decode only consumes the
    // first; the second must be drained before sideband demux or the demuxer
    // misreads "NAK\n" as a sideband frame with channel 'N'.
    // This simulates that double-NAK followed by a sideband-wrapped PACK header
    // (channel 0x01 + "PACK" magic).
    payload := []byte("0008NAK\n0008NAK\n0009\x01PACK")
    ep, err := transport.NewEndpoint("https://example.com/repo.git")
    if err != nil {
        t.Fatalf("parse endpoint: %v", err)
    }
    body := &trackingReadCloser{ReadCloser: io.NopCloser(bytes.NewReader(payload))}
    conn := NewConn(ep, "source", nil, roundTripperFunc(func(req *http.Request) (*http.Response, error) {
        return &http.Response{
            StatusCode: http.StatusOK,
            Request:    req,
            Body:       body,
        }, nil
    }))

adv := packp.NewAdvRefs()
    _ = adv.Capabilities.Set(capability.Sideband64k)
    desired := map[plumbing.ReferenceName]DesiredRef{
        plumbing.NewBranchReferenceName("main"): {
            SourceRef:  plumbing.NewBranchReferenceName("main"),
            TargetRef:  plumbing.NewBranchReferenceName("main"),
            SourceHash: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
        },
    }

rc, err := fetchPackV1(context.Background(), conn, adv, desired, nil)
    if err != nil {
        t.Fatalf("fetchPackV1 should tolerate double-NAK preamble, got: %v", err)
    }
    got, err := io.ReadAll(rc)
    // Reader will error (or hit EOF) after the truncated sideband frame; what
    // we care about is that the demuxed payload started with PACK, proving the
    // second NAK was drained rather than fed into the demuxer.
    if !bytes.HasPrefix(got, []byte("PACK")) {
        t.Fatalf("expected demuxed pack bytes to start with PACK, got %q (err=%v)", got, err)
    }
    if closeErr := rc.Close(); closeErr != nil {
        t.Fatalf("close returned reader: %v", closeErr)
    }
    if !body.closed {
        t.Fatal("expected returned reader close to close underlying body")
    }
}

func TestFetchPackV2ClosesBodyOnDecodeError(t *testing.T) {
    ep, err := transport.NewEndpoint("https://example.com/repo.git")
    if err != nil {