fix upload-pack negotiation for stateless-RPC

Three related bugs in the git smart-HTTP upload-pack handler surfaced as
'bad band #78', 60s 504 hangs, and "expected ACK/NAK, got '?PACK'" when
fetching after previous pulls.

- Don't advertise no-done; we don't implement the ACK ready handshake.
- With multi_ack_detailed, don't emit per-round NAKs — only the final
  one after 'done'. Per-round NAKs make the client treat the first as
  the transition to sideband pack data, then choke on the second.
- Break out of the negotiation loop when a round reads no content
  (client closed without 'done') so we don't spin on EOF.
- Only emit pack data when 'done' was actually received. Non-final
  rounds get just NAK so HTTP keep-alive doesn't desync the next round.

Also trigger null_resource.test on the ko_build image digest so tests
re-run on every deploy.

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

apps.tf

@@ -5,21 +5,18 @@ locals {
       ram                   = "512Mi"
       container_concurrency = 1
       timeout_seconds       = 900 # 15m
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
     }
     "flatten" : {
       cpu                   = 1
       ram                   = "1Gi"
       container_concurrency = 80
       timeout_seconds       = 120 # 2m
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
     }
     "git" : {
       cpu                   = 1
       ram                   = "2Gi"
       container_concurrency = 5
       timeout_seconds       = 900 # 15m
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
     }
     "ko" : {
       cpu                   = 2
@@ -33,14 +30,12 @@ locals {
       ram                   = "4Gi"
       container_concurrency = 1
       timeout_seconds       = 900 # 15m
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
     }
     "random" : {
       cpu                   = 1
       ram                   = "256Mi"
       container_concurrency = 1000
       timeout_seconds       = 60 # 1m
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
       //alert_id              = module.prober.alert_id
     }
     wait : {
@@ -48,20 +43,23 @@ locals {
       ram                   = "1Gi"
       container_concurrency = 80
       timeout_seconds       = 60 # 1m
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
     }
+    # "viz" : {
+    #   cpu                   = 1
+    #   ram                   = "1Gi"
+    #   container_concurrency = 1000
+    #   timeout_seconds       = 60 # 1m
+    # }
     "infinite-git" : {
       cpu                   = 1
       ram                   = "1Gi"
       container_concurrency = 1000
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
       timeout_seconds       = 60 # 1m
     }
     "infinite-go" : {
       cpu                   = 1
       ram                   = "1Gi"
       container_concurrency = 1000
-      base_image            = "cgr.dev/chainguard/static:latest-glibc"
       timeout_seconds       = 60 # 1m
     }
   }
@@ -72,7 +70,7 @@ resource "ko_build" "image" {
 
   repo       = "gcr.io/${var.project_id}/${each.key}"
   importpath = "github.com/imjasonh/kontain.me/cmd/${each.key}"
-  base_image = each.value.base_image
+  base_image = lookup(each.value, "base_image", "cgr.dev/chainguard/static:latest")
 }
 
 resource "google_cloud_run_service" "service" {
@@ -151,8 +149,8 @@ resource "null_resource" "test" {
   for_each   = local.apps
   depends_on = [google_dns_record_set.dns-record]
 
-  # Changes to any service requires re-testing it.
+  # Re-run tests whenever the deployed image changes.
-  triggers = { service = google_cloud_run_service.service[each.key].id }
+  triggers = { image = ko_build.image[each.key].image_ref }
 
   provisioner "local-exec" { command = file("${path.module}/cmd/${each.key}/test.sh") }
 }

cmd/git/test.sh

@@ -6,5 +6,5 @@ time crane validate --remote=git.kontain.me/github.com/imjasonh/kontain.me:main
 # Default tag resolves to the default branch (main): same commit, cache hit.
 time crane validate --remote=git.kontain.me/github.com/imjasonh/kontain.me
 
-docker run --rm git.kontain.me/github.com/imjasonh/kontain.me -c "git log -1"
+docker run --rm --pull=always git.kontain.me/github.com/imjasonh/kontain.me -c "git log -1"
-docker run --rm git.kontain.me/github.com/google/go-containerregistry -c "git fetch --unshallow && git log"
+docker run --rm --pull=always git.kontain.me/github.com/google/go-containerregistry -c "git fetch --unshallow && git log"

cmd/infinite-go/main_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"io"
 	"net"
 	"net/http"
 	"net/http/httptest"
@@ -10,6 +11,7 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/imjasonh/kontain.me/internal/repo"
 	"github.com/imjasonh/kontain.me/internal/server"
@@ -132,6 +134,203 @@ func TestGoGetE2E(t *testing.T) {
 	}
 }
 
+// TestUploadPackNoDoneTerminates verifies the handler doesn't hang when the
+// client ends the request without "done". This is one round of stateless-RPC
+// negotiation: the client sends haves and a flush, expecting just a NAK in
+// response so it can decide what to send next. Sending pack data here would
+// corrupt the HTTP keep-alive connection for the follow-up "done" round.
+func TestUploadPackNoDoneTerminates(t *testing.T) {
+	ts := newGoTestServer(t, "example.com/infinite-go")
+
+	resp, err := http.Get(ts.URL + "/info/refs?service=git-upload-pack")
+	if err != nil {
+		t.Fatal(err)
+	}
+	infoBody, _ := io.ReadAll(resp.Body)
+	resp.Body.Close()
+
+	var sha string
+	for i := 0; i+40 <= len(infoBody); i++ {
+		ok := true
+		for j := 0; j < 40; j++ {
+			c := infoBody[i+j]
+			if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) {
+				ok = false
+				break
+			}
+		}
+		if ok && (i+40 == len(infoBody) || infoBody[i+40] == ' ') {
+			sha = string(infoBody[i : i+40])
+			break
+		}
+	}
+	if sha == "" {
+		t.Fatalf("could not parse SHA from info/refs")
+	}
+
+	pkt := func(s string) string { return fmt.Sprintf("%04x%s", len(s)+4, s) }
+	// Request shaped exactly like a real git fetch using no-done: wants,
+	// flush, haves, double flush, NO "done".
+	req := pkt("want "+sha+" multi_ack_detailed no-done side-band-64k ofs-delta agent=test\n") +
+		"0000" +
+		pkt("have 1111111111111111111111111111111111111111\n") +
+		pkt("have 2222222222222222222222222222222222222222\n") +
+		"00000000"
+
+	done := make(chan struct{})
+	var body []byte
+	var status int
+	go func() {
+		defer close(done)
+		r, err := http.Post(ts.URL+"/git-upload-pack",
+			"application/x-git-upload-pack-request", strings.NewReader(req))
+		if err != nil {
+			t.Errorf("POST: %v", err)
+			return
+		}
+		body, _ = io.ReadAll(r.Body)
+		r.Body.Close()
+		status = r.StatusCode
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(10 * time.Second):
+		t.Fatal("handler hung past 10s — no-done termination not handled")
+	}
+	if status != 200 {
+		t.Fatalf("status %d, want 200; body: %q", status, body)
+	}
+	if string(body) != "0008NAK\n" {
+		t.Errorf("want NAK-only response (no pack data); got %q", body)
+	}
+}
+
+// TestUploadPackMultiBatchHaves drives the upload-pack handler directly with
+// two flush-separated batches of haves the server doesn't know, then verifies
+// that the response contains exactly one NAK (not two). Two NAKs would cause
+// the client to interpret the second as the start of sideband-framed pack data
+// and choke on band byte 'N' (0x4E=78) — "fetch-pack: protocol error: bad band #78".
+func TestUploadPackMultiBatchHaves(t *testing.T) {
+	ts := newGoTestServer(t, "example.com/infinite-go")
+
+	resp, err := http.Get(ts.URL + "/info/refs?service=git-upload-pack")
+	if err != nil {
+		t.Fatal(err)
+	}
+	infoBody, _ := io.ReadAll(resp.Body)
+	resp.Body.Close()
+
+	var sha string
+	for i := 0; i+40 <= len(infoBody); i++ {
+		ok := true
+		for j := 0; j < 40; j++ {
+			c := infoBody[i+j]
+			if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) {
+				ok = false
+				break
+			}
+		}
+		if ok && (i+40 == len(infoBody) || infoBody[i+40] == ' ') {
+			sha = string(infoBody[i : i+40])
+			break
+		}
+	}
+	if sha == "" {
+		t.Fatalf("could not parse SHA from info/refs response")
+	}
+
+	pkt := func(s string) string { return fmt.Sprintf("%04x%s", len(s)+4, s) }
+	req := pkt("want "+sha+" multi_ack_detailed side-band-64k ofs-delta agent=test\n") +
+		"0000" +
+		pkt("have 1111111111111111111111111111111111111111\n") +
+		"0000" +
+		pkt("have 2222222222222222222222222222222222222222\n") +
+		pkt("done\n")
+
+	resp, err = http.Post(ts.URL+"/git-upload-pack",
+		"application/x-git-upload-pack-request", strings.NewReader(req))
+	if err != nil {
+		t.Fatal(err)
+	}
+	body, _ := io.ReadAll(resp.Body)
+	resp.Body.Close()
+
+	naks := 0
+	off := 0
+	for off+4 <= len(body) {
+		var length int
+		fmt.Sscanf(string(body[off:off+4]), "%04x", &length)
+		if length == 0 {
+			off += 4
+			continue
+		}
+		end := off + length
+		if end > len(body) {
+			break
+		}
+		content := body[off+4 : end]
+		if string(content) == "NAK\n" {
+			naks++
+		}
+		if len(content) > 0 && content[0] == 0x01 {
+			break // entered sideband pack stream
+		}
+		off = end
+	}
+	if naks != 1 {
+		t.Errorf("expected exactly 1 NAK before pack data, got %d", naks)
+	}
+}
+
+// TestFetchWithUnknownHaves simulates a client whose local cache contains
+// commits the server has never seen — e.g. a fresh Cloud Run instance after a
+// previous instance generated different commits. The client sends "have" lines
+// the server can't ACK; the server must still serve a valid sideband-framed
+// packfile rather than confusing the client (previously: "bad band #78").
+func TestFetchWithUnknownHaves(t *testing.T) {
+	gitBin, err := exec.LookPath("git")
+	if err != nil {
+		t.Skip("git binary not found in PATH")
+	}
+
+	// Clone from a first server, then fetch from a second (fresh) server.
+	// The client's cache will contain commits the second server has never
+	// seen — this is the case that triggers "bad band #78".
+	ts1 := newGoTestServer(t, "example.com/infinite-go")
+
+	cacheDir := t.TempDir()
+	cmd := exec.Command(gitBin, "init", "--bare", "-b", "main", cacheDir)
+	if out, err := cmd.CombinedOutput(); err != nil {
+		t.Fatalf("git init: %v\n%s", err, out)
+	}
+
+	runGit := func(args ...string) {
+		t.Helper()
+		cmd := exec.Command(gitBin, append([]string{"-C", cacheDir}, args...)...)
+		if out, err := cmd.CombinedOutput(); err != nil {
+			t.Fatalf("git %v: %v\n%s", args, err, out)
+		}
+	}
+	runGit("remote", "add", "origin", ts1.URL)
+	// Build up enough history that the client sends "have" lines in
+	// multiple flush-separated rounds on the next fetch — closer to a
+	// real Cloud Run instance restart after many pulls.
+	for i := 0; i < 3; i++ {
+		runGit("fetch", "origin", "+refs/heads/*:refs/remotes/origin/*")
+	}
+
+	// Now point at a fresh server with no shared history and fetch again.
+	ts2 := newGoTestServer(t, "example.com/infinite-go")
+	runGit("remote", "set-url", "origin", ts2.URL)
+
+	cmd = exec.Command(gitBin, "-C", cacheDir, "fetch", "-f", "--end-of-options",
+		"origin", "refs/heads/*:refs/heads/*", "refs/tags/*:refs/tags/*")
+	if out, err := cmd.CombinedOutput(); err != nil {
+		t.Fatalf("git fetch failed: %v\n%s", err, out)
+	}
+}
+
 // TestGoGetPull clones and then pulls repeatedly using the git CLI,
 // verifying each pull produces a buildable Go package with a unique PullTime.
 // This always runs (no privileged port needed) as a fallback for environments

cmd/infinite-go/test.sh

@@ -23,5 +23,5 @@ if [ "$before" = "$after" ]; then
     exit 1
 fi
 
-go get infinite-git.kontain.me@latest
+GOPROXY=direct go get infinite-git.kontain.me@latest
-go get infinite-git.kontain.me@latest
+GOPROXY=direct go get infinite-git.kontain.me@latest

go.mod

@@ -189,6 +189,7 @@ require (
 	gopkg.in/ini.v1 v1.67.2 // indirect
 	gopkg.in/warnings.v0 v0.1.2 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
+	infinite-go.kontain.me v0.0.0-20260514014836-7342d5554691 // indirect
 	k8s.io/apimachinery v0.36.0 // indirect
 	k8s.io/klog/v2 v2.140.0 // indirect
 	sigs.k8s.io/release-utils v0.12.4 // indirect

go.sum

@@ -1016,6 +1016,10 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
 honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
+infinite-go.kontain.me v0.0.0-20260514014826-809726c2f439 h1:bLBaQ2s/AHqVJ7P7rAIrGndJsnVBKrblwUNcV1N5XVE=
+infinite-go.kontain.me v0.0.0-20260514014826-809726c2f439/go.mod h1:jCDExCTcYpSmadEboALxkad3O9//SfK7LjKg19aODSI=
+infinite-go.kontain.me v0.0.0-20260514014836-7342d5554691 h1:VV3xRzFhtxGWTL8WvXnWRnp8iT2CGiOdZAc0Doso6uQ=
+infinite-go.kontain.me v0.0.0-20260514014836-7342d5554691/go.mod h1:jCDExCTcYpSmadEboALxkad3O9//SfK7LjKg19aODSI=
 k8s.io/apimachinery v0.36.0 h1:jZyPzhd5Z+3h9vJLt0z9XdzW9VzNzWAUw+P1xZ9PXtQ=
 k8s.io/apimachinery v0.36.0/go.mod h1:FklypaRJt6n5wUIwWXIP6GJlIpUizTgfo1T/As+Tyxc=
 k8s.io/klog/v2 v2.140.0 h1:Tf+J3AH7xnUzZyVVXhTgGhEKnFqye14aadWv7bzXdzc=

internal/protocol/upload_pack.go

@@ -53,15 +53,31 @@ func (u *UploadPack) HandleRequest(r io.Reader, w io.Writer) error {
 		}
 	}
 
+	// Detect multi_ack mode from advertised client capabilities. With
+	// multi_ack_detailed (the modern default), we send "ACK <sha> common"
+	// per match and a single final ACK/NAK after "done" — no per-round
+	// NAK. With plain multi_ack we send "ACK <sha> continue" and a NAK
+	// per round with no ACKs. Without either capability the protocol
+	// expects at most one ACK before "done" plus a final ACK/NAK.
+	multiAck, multiAckDetailed := false, false
+	for _, cap := range capabilities {
+		switch cap {
+		case "multi_ack":
+			multiAck = true
+		case "multi_ack_detailed":
+			multiAckDetailed = true
+		}
+	}
+
 	// Negotiation phase: the client sends "have" lines for objects it
 	// already has, and we ACK the ones we recognize so git can compute a
-	// minimal pack. With multi_ack_detailed we send "ACK <sha> common"
+	// minimal pack.
-	// for each recognized object.
 	var common []string
+	gotDone := false
 
 	for {
 		var haves []string
-		gotDone := false
+		sawAny := false
 
 		for {
 			line, err := reader.ReadString()
@@ -71,6 +87,7 @@ func (u *UploadPack) HandleRequest(r io.Reader, w io.Writer) error {
 			if err != nil {
 				return fmt.Errorf("reading negotiation: %w", err)
 			}
+			sawAny = true
 
 			if line == "done" {
 				gotDone = true
@@ -82,15 +99,27 @@ func (u *UploadPack) HandleRequest(r io.Reader, w io.Writer) error {
 			}
 		}
 
-		// ACK objects we have, NAK the rest. Process haves even when
+		// A round with no content at all means the client closed the
-		// "done" arrived in the same batch — the client needs ACKs
+		// negotiation without sending "done" (e.g. with the no-done
-		// before we send the packfile.
+		// capability, or simply ran out of haves). Treat it as the end
+		// of negotiation so we don't spin reading EOF forever.
+		if !sawAny && !gotDone {
+			break
+		}
+
+		// ACK objects we have. Process haves even when "done" arrived
+		// in the same batch — the client needs ACKs before we send the
+		// packfile.
 		var lastAck string
 		for _, have := range haves {
 			if _, err := u.repo.ReadObjectFull(have); err == nil {
 				common = append(common, have)
 				lastAck = have
-				if err := writer.WriteString(fmt.Sprintf("ACK %s common\n", have)); err != nil {
+				ackType := "common"
+				if !multiAckDetailed {
+					ackType = "continue"
+				}
+				if err := writer.WriteString(fmt.Sprintf("ACK %s %s\n", have, ackType)); err != nil {
 					return fmt.Errorf("writing ACK: %w", err)
 				}
 			}
@@ -100,25 +129,33 @@ func (u *UploadPack) HandleRequest(r io.Reader, w io.Writer) error {
 			break
 		}
 
-		if lastAck != "" {
+		// Per-round NAK is only sent for plain multi_ack. With
-			if err := writer.WriteString(fmt.Sprintf("ACK %s continue\n", lastAck)); err != nil {
+		// multi_ack_detailed there is no per-round NAK — sending one
-				return fmt.Errorf("writing ACK continue: %w", err)
+		// would confuse the client into thinking the final ACK/NAK
-			}
+		// (and the start of pack data) has arrived.
-		} else {
+		if multiAck && !multiAckDetailed && lastAck == "" {
 			if err := writer.WriteString("NAK\n"); err != nil {
 				return fmt.Errorf("writing NAK: %w", err)
 			}
 		}
 	}
 
-	// Try to read the flush after "done". The client may or may not send
+	// Drain any remaining trailing flush after "done" so the request body
-	// one, and the body may already be closed (especially behind proxies).
+	// is fully consumed.
 	if _, err := reader.ReadString(); err != nil && err != io.EOF {
-		// Ignore read errors on closed body — the client is done sending.
+		// Ignore — body may be closed.
 	}
 
-	// After "done", send a final ACK for the last common object (or NAK
+	// Stateless-RPC semantics: when the client ends the request without
-	// if there are no common objects, e.g. initial clone).
+	// "done", this is a negotiation round, not the final fetch. Reply
+	// with NAK only (no pack data) so the client can send another round
+	// with more haves. Emitting pack data here would corrupt the
+	// keep-alive connection for the next round.
+	if !gotDone {
+		return writer.WriteString("NAK\n")
+	}
+
+	// Final response after "done": ACK <last common> or NAK, then pack.
 	if len(common) > 0 {
 		if err := writer.WriteString(fmt.Sprintf("ACK %s\n", common[len(common)-1])); err != nil {
 			return fmt.Errorf("writing final ACK: %w", err)

internal/repo/repo.go

@@ -229,7 +229,6 @@ func (r *Repository) GetCapabilities() []string {
 		"no-progress",
 		"include-tag",
 		"multi_ack_detailed",
-		"no-done",
 		"symref=HEAD:refs/heads/main",
 		"agent=infinite-git/1.0",
 	}