fix(perforce): truncate on max_bytes instead of failing, and size the P-class bounds for real changelists

internal/tools/perforce.go

@@ -53,6 +53,12 @@ type Describe struct {
 	Status      string          `json:"status"`
 	Files       []DescribedFile `json:"files"`
 	FileCount   int             `json:"file_count"`
+
+	// The changelist's real file count, before max_files or max_bytes were
+	// applied. FileCount is what was RETURNED; this is what EXISTS. A consumer
+	// comparing the two can see exactly how much a truncated record is missing
+	// instead of only knowing that it is short (#1904).
+	DepotFileCount int `json:"depot_file_count"`
 }
 
 // ChangeSummary is one entry of p4.changes.
@@ -82,14 +88,28 @@ func (p *Perforce) Execute(ctx context.Context, verb string, args map[string]any
 func (p *Perforce) describe(ctx context.Context, change int64, maxFiles, maxBytes int) (any, int, bool, error) {
 	// -s omits the diffs entirely; this is the content boundary enforced at the
 	// tool invocation, not only in the schema.
-	recs, n, err := p.run(ctx, maxBytes, "describe", "-s", strconv.FormatInt(change, 10))
+	recs, n, err := p.run(ctx, "describe", "-s", strconv.FormatInt(change, 10))
 	if err != nil {
 		return nil, n, false, err
 	}
 	if len(recs) == 0 {
 		return nil, n, false, fmt.Errorf("perforce: changelist %d not found", change)
 	}
-	r := recs[0]
+	out, truncated := describeFromRecord(recs[0], change, maxFiles, maxBytes)
+	return out, n, truncated, nil
+}
+
+// describeFromRecord shapes one p4 -Mj describe record into a Describe,
+// applying both size bounds. Split out from describe() so the bounds - which
+// are the whole subject of #1904 - can be exercised without a live p4 server.
+//
+// TWO bounds apply, and both TRUNCATE rather than fail (PROTOCOL.md,
+// "Ordering, replay, and size": "the connector truncates and sets
+// truncated: true rather than streaming unbounded"). max_bytes used to
+// short-circuit in run() with an error instead, which is why changelist 118 on
+// project 108 - 503 files, an ordinary asset reorganisation for a game studio -
+// never synced at all rather than syncing partially.
+func describeFromRecord(r map[string]any, change int64, maxFiles, maxBytes int) (Describe, bool) {
 	out := Describe{
 		Change:      change,
 		User:        str(r, "user"),
@@ -98,30 +118,70 @@ func (p *Perforce) describe(ctx context.Context, change int64, maxFiles, maxByte
 		Description: str(r, "desc"),
 		Status:      str(r, "status"),
 	}
+
 	// p4 -Mj returns indexed keys: depotFile0, action0, type0, rev0, ...
+	//
+	// depotFileCount is the changelist's REAL file count, counted before either
+	// bound is applied. Without it a truncated record cannot be told from a
+	// complete one by size alone, and the consumer has no way to know how much
+	// it is missing - FileCount below is only what was returned.
 	truncated := false
+	budget := maxBytes
+	if budget <= 0 {
+		budget = DefaultDescribeMaxBytes
+	}
+
+	depotFileCount := 0
 	for i := 0; ; i++ {
 		df := str(r, "depotFile"+strconv.Itoa(i))
 		if df == "" {
 			break
 		}
+		depotFileCount++
+
+		if truncated {
+			continue // keep counting; stop collecting
+		}
 		if len(out.Files) >= maxFiles {
 			truncated = true
-			break
+			continue
 		}
-		out.Files = append(out.Files, DescribedFile{
+
+		file := DescribedFile{
 			DepotFile: df,
 			Action:    str(r, "action"+strconv.Itoa(i)),
 			Type:      str(r, "type"+strconv.Itoa(i)),
 			Rev:       str(r, "rev"+strconv.Itoa(i)),
-		})
+		}
+		// Charged against the budget before appending, so the bound holds
+		// rather than being discovered one record after it was crossed.
+		cost := file.approxJSONBytes()
+		if cost > budget {
+			truncated = true
+			continue
+		}
+		budget -= cost
+
+		out.Files = append(out.Files, file)
 	}
 	out.FileCount = len(out.Files)
-	return out, n, truncated, nil
+	out.DepotFileCount = depotFileCount
+	return out, truncated
+}
+
+// approxJSONBytes estimates what this record costs in the encoded response.
+// An estimate is the right tool: the exact cost depends on the encoder's
+// escaping, and re-marshalling every record to find out would make the bound
+// more expensive than the work it bounds. It counts the field values plus a
+// fixed allowance for the keys, braces, quotes and commas, and it rounds UP,
+// so the real response is never larger than the budget implies.
+func (f DescribedFile) approxJSONBytes() int {
+	const envelope = 64 // {"depot_file":"","action":"","type":"","rev":""},
+	return envelope + len(f.DepotFile) + len(f.Action) + len(f.Type) + len(f.Rev)
 }
 
 func (p *Perforce) changes(ctx context.Context, path string, max, maxBytes int) (any, int, bool, error) {
-	recs, n, err := p.run(ctx, maxBytes, "changes", "-m", strconv.Itoa(max), path)
+	recs, n, err := p.run(ctx, "changes", "-m", strconv.Itoa(max), path)
 	if err != nil {
 		return nil, n, false, err
 	}
@@ -138,9 +198,60 @@ func (p *Perforce) changes(ctx context.Context, path string, max, maxBytes int)
 	return out, n, false, nil
 }
 
+// Size bounds.
+//
+// These used to be metadata-sized (64 KiB, roughly 500 file records) and they
+// decided which changelists were allowed to be CORRECT rather than protecting
+// anything: ButterStack already stores the full file list for every changelist
+// under the limit, so the cap never stopped the data existing in the product.
+// An asset reorganisation of a few hundred files is ordinary for a game studio
+// and is exactly what this daemon exists to observe.
+//
+// So the defaults are now sized for the work, and the true ceiling is a
+// separate, much larger memory bound.
+const (
+	// DefaultDescribeMaxBytes is the response budget for p4.describe when the
+	// caller names none. ~4 MiB is roughly 30,000 file records: past any real
+	// changelist, well short of a memory problem.
+	DefaultDescribeMaxBytes = 4 << 20
+
+	// MaxToolOutputBytes is how much raw p4 output the daemon will hold for a
+	// single command, so a pathological changelist cannot exhaust it. This is
+	// the sanity bound; it is not a response-shaping knob and no caller can
+	// raise it.
+	MaxToolOutputBytes = 64 << 20
+)
+
+// cappedBuffer is a bytes.Buffer that stops growing past limit and records
+// that it did. Writing into a plain buffer and checking its length afterwards
+// would already have allocated whatever p4 produced, which is the allocation
+// the ceiling exists to prevent.
+type cappedBuffer struct {
+	buf        bytes.Buffer
+	limit      int
+	overflowed bool
+}
+
+func (c *cappedBuffer) Write(p []byte) (int, error) {
+	if c.overflowed {
+		return len(p), nil // absorb the rest; the command is already doomed
+	}
+	if remaining := c.limit - c.buf.Len(); len(p) > remaining {
+		c.overflowed = true
+		if remaining > 0 {
+			c.buf.Write(p[:remaining])
+		}
+		return len(p), nil
+	}
+	return c.buf.Write(p)
+}
+
+func (c *cappedBuffer) Bytes() []byte { return c.buf.Bytes() }
+func (c *cappedBuffer) Len() int      { return c.buf.Len() }
+
 // run invokes p4 with -Mj (one JSON object per record) and returns the parsed
 // records. args is appended to a fixed prefix; nothing in args is interpreted.
-func (p *Perforce) run(ctx context.Context, maxBytes int, args ...string) ([]map[string]any, int, error) {
+func (p *Perforce) run(ctx context.Context, args ...string) ([]map[string]any, int, error) {
 	argv := append([]string{
 		"-p", p.cfg.Port,
 		"-u", p.cfg.User,
@@ -152,26 +263,42 @@ func (p *Perforce) run(ctx context.Context, maxBytes int, args ...string) ([]map
 
 	cmd := exec.CommandContext(ctx, p.cfg.Binary, argv...) // argv array; no shell
 	cmd.Env = p.env()
-	var stdout, stderr bytes.Buffer
-	cmd.Stdout, cmd.Stderr = &stdout, &stderr
+	// The per-verb max_bytes budget is NOT enforced here. It is a response-
+	// shaping bound and belongs where records are assembled, so a large
+	// changelist truncates with truncated: true rather than failing outright.
+	// What is enforced here is a memory ceiling: a bound on what the daemon is
+	// willing to hold, not on what the caller asked for. It is measured in
+	// megabytes and a changelist that trips it is genuinely pathological.
+	stdout := &cappedBuffer{limit: MaxToolOutputBytes}
+	var stderr bytes.Buffer
+	cmd.Stdout, cmd.Stderr = stdout, &stderr
 
 	if err := cmd.Run(); err != nil {
 		return nil, stdout.Len(), fmt.Errorf("perforce: %s", failureDetail(stdout.Bytes(), stderr.String(), err))
 	}
-	if maxBytes > 0 && stdout.Len() > maxBytes {
-		return nil, stdout.Len(), fmt.Errorf("perforce: response exceeded max_bytes")
+	if stdout.overflowed {
+		return nil, stdout.Len(), fmt.Errorf("perforce: output exceeded the %d-byte tool ceiling", MaxToolOutputBytes)
 	}
 
+	// Decode from a reader over the bytes rather than from the buffer itself.
+	// Decoding CONSUMES a bytes.Buffer, so the previous `json.NewDecoder(&stdout)`
+	// left it empty and the `stdout.Len()` reported below was therefore always
+	// 0 - every successful command reported `bytes: 0` to the broker and into
+	// the audit log. Found while removing the max_bytes hard-fail above, which
+	// was the only caller that read the length before the decode drained it.
+	raw := stdout.Bytes()
+	n := len(raw)
+
 	var recs []map[string]any
-	dec := json.NewDecoder(&stdout)
+	dec := json.NewDecoder(bytes.NewReader(raw))
 	for {
 		var m map[string]any
 		if err := dec.Decode(&m); err != nil {
 			break
 		}
 		recs = append(recs, m)
 	}
-	return recs, stdout.Len(), nil
+	return recs, n, nil
 }
 
 // env builds a minimal environment. The ticket goes in P4PASSWD rather than on

internal/tools/perforce_describe_test.go

@@ -0,0 +1,147 @@
+package tools
+
+import (
+	"strconv"
+	"testing"
+)
+
+// #1904: max_bytes used to short-circuit with an error instead of truncating,
+// contradicting PROTOCOL.md ("the connector truncates and sets truncated: true
+// rather than streaming unbounded"). That is why changelist 118 on project 108
+// - 503 files, an ordinary asset reorganisation - never synced at all rather
+// than syncing partially.
+//
+// describe() reads a p4 -Mj record, so these drive it through that record shape
+// rather than through a live p4.
+func describeRecord(fileCount int) map[string]any {
+	r := map[string]any{
+		"change": "118",
+		"user":   "ryan",
+		"client": "bsg-cp-01",
+		"time":   "1757260800",
+		"desc":   "audio stack, asset reorg",
+		"status": "submitted",
+	}
+	for i := 0; i < fileCount; i++ {
+		r["depotFile"+strconv.Itoa(i)] = "//pilot_light/art/texture_" + strconv.Itoa(i) + ".png"
+		r["action"+strconv.Itoa(i)] = "add"
+		r["type"+strconv.Itoa(i)] = "binary"
+		r["rev"+strconv.Itoa(i)] = "1"
+	}
+	return r
+}
+
+func collect(t *testing.T, fileCount, maxFiles, maxBytes int) (Describe, bool) {
+	t.Helper()
+	out, truncated := describeFromRecord(describeRecord(fileCount), 118, maxFiles, maxBytes)
+	return out, truncated
+}
+
+func TestDescribeTruncatesOnMaxFiles(t *testing.T) {
+	out, truncated := collect(t, 503, 200, DefaultDescribeMaxBytes)
+
+	if !truncated {
+		t.Fatal("a file list cut short by max_files must report truncated")
+	}
+	if out.FileCount != 200 {
+		t.Errorf("FileCount = %d, want 200 (what was returned)", out.FileCount)
+	}
+	if out.DepotFileCount != 503 {
+		t.Errorf("DepotFileCount = %d, want 503 (what exists) - without this a consumer cannot tell how much is missing", out.DepotFileCount)
+	}
+}
+
+// The behaviour change this issue is about: a byte budget shapes the response,
+// it does not fail the command.
+func TestDescribeTruncatesOnMaxBytesRatherThanFailing(t *testing.T) {
+	out, truncated := collect(t, 503, 100000, 4096)
+
+	if !truncated {
+		t.Fatal("a file list cut short by max_bytes must report truncated, not error")
+	}
+	if out.FileCount == 0 {
+		t.Fatal("a tight byte budget must still return the records that fit, not nothing")
+	}
+	if out.FileCount >= 503 {
+		t.Fatalf("FileCount = %d: the byte budget was not applied", out.FileCount)
+	}
+	if out.DepotFileCount != 503 {
+		t.Errorf("DepotFileCount = %d, want 503", out.DepotFileCount)
+	}
+}
+
+// Changelist 118's own shape. Under the old 65536 cap this was the failing
+// case; it must now come back whole.
+func TestDescribeReturnsAFiveHundredFileChangelistWhole(t *testing.T) {
+	out, truncated := collect(t, 503, 100000, DefaultDescribeMaxBytes)
+
+	if truncated {
+		t.Error("503 files is an ordinary changelist and must not truncate at the default budget")
+	}
+	if out.FileCount != 503 {
+		t.Errorf("FileCount = %d, want 503", out.FileCount)
+	}
+	if out.DepotFileCount != out.FileCount {
+		t.Errorf("a complete record must have DepotFileCount == FileCount, got %d vs %d", out.DepotFileCount, out.FileCount)
+	}
+}
+
+func TestDescribeCompleteListIsNotMarkedTruncated(t *testing.T) {
+	out, truncated := collect(t, 12, 200, DefaultDescribeMaxBytes)
+
+	if truncated {
+		t.Error("a list under both bounds must not be marked truncated")
+	}
+	if out.FileCount != 12 || out.DepotFileCount != 12 {
+		t.Errorf("FileCount/DepotFileCount = %d/%d, want 12/12", out.FileCount, out.DepotFileCount)
+	}
+}
+
+// The budget must hold, not be discovered one record after it was crossed.
+func TestDescribeStaysWithinItsByteBudget(t *testing.T) {
+	const budget = 8192
+	out, _ := collect(t, 503, 100000, budget)
+
+	total := 0
+	for _, f := range out.Files {
+		total += f.approxJSONBytes()
+	}
+	if total > budget {
+		t.Errorf("returned records cost %d bytes against a %d budget", total, budget)
+	}
+}
+
+// A zero budget means "no caller preference", not "return nothing".
+func TestDescribeZeroMaxBytesUsesTheDefault(t *testing.T) {
+	out, truncated := collect(t, 503, 100000, 0)
+
+	if truncated || out.FileCount != 503 {
+		t.Errorf("max_bytes=0 should fall back to the default budget; got truncated=%v FileCount=%d", truncated, out.FileCount)
+	}
+}
+
+func TestCappedBufferStopsAtItsLimit(t *testing.T) {
+	b := &cappedBuffer{limit: 10}
+
+	n, err := b.Write([]byte("12345"))
+	if n != 5 || err != nil || b.overflowed {
+		t.Fatalf("under the limit: n=%d err=%v overflowed=%v", n, err, b.overflowed)
+	}
+
+	// Writes past the limit are absorbed, not errored: the command is already
+	// doomed and returning an error here would surface as a write failure
+	// rather than as the size problem it is.
+	n, err = b.Write([]byte("678901234567890"))
+	if err != nil {
+		t.Fatalf("a write past the limit must be absorbed, got %v", err)
+	}
+	if n != 15 {
+		t.Errorf("Write must report the full length it was given, got %d", n)
+	}
+	if !b.overflowed {
+		t.Error("overflow must be recorded")
+	}
+	if b.Len() != 10 {
+		t.Errorf("buffer grew to %d, past its %d limit - the allocation the ceiling exists to prevent", b.Len(), 10)
+	}
+}